Refactoring of geocache attributes #2304
Conversation
This method uses HTMLPurifier to clean up input text with html tags but this is take a lot of memory. OCCookie can use just htmlspecialchars instead (it saves a half of the memory of each OCPL code call as asmost always OCCookie is in use.
kojoty
force-pushed
the
pju-attributes
branch
2 times, most recently
from
676629a to
4341c00
Compare
|
This is great news. I've been working on attribute definitions the previous years (dataset) a lot and also created an attribute set (graphics). I paused because of the code changes it required. Please include attribute theme configuration directive. Also note that OC UK has requested at least one new attribute (I had extensive talks with them in 2019) and the removal of several. I'll look it up and post it. My work also included opencaching.eu documentation and a partial redefining of the attribute IDs from the current mess to a consistent, cross node uniform set. IIRC I developed some SQL queries to help make the transition for cache - attribute mappings. Don't forget OC US has some particular attribs as well, since they dropped some cache types in favour of attributes. |
|
I'll recheck my attribute sheets and update the wiki. |
|
Checking https://wiki.opencaching.eu/index.php?title=Cache_attributes against my master table. I think I have to update it with several of the OCUS attribs (derived from cache type fallback).
Also, OCUK would like to add at least new attrib 217. Before applying SQL updates on data (per note designed SQL queries) I think it best that I recheck them. (also in light with OCUS changes) |
|
Reading the code looks like I should make additional updates to the wiki (and my tables). |
|
From Facebook UK technical group:
@kojoty could you check if any OC UK caches has these attributes? |
config/geocache.default.php
Outdated
|
|
||
| /** | ||
| * List of attributes supported by the node. The order is significant - the same order is used on sites. | ||
| * Use CacheAttribute::AT_* notation for more clear definition. |
There was a problem hiding this comment.
There is no AT_* notation in CacheAttribute class.
| @@ -3136,6 +3136,86 @@ | |||
| 'vote_elListThEnd' => 'End of election', | |||
| 'vote_optionsToChoose' => 'Options to choose from', | |||
|
|
|||
|
|
|||
| 'at_fee' => 'Access or parking fee', | |||
There was a problem hiding this comment.
I think it is not clear. I suggest rename it to attr_*
There was a problem hiding this comment.
I don't know - this is just a prefix. Shorter is easier to write...
There was a problem hiding this comment.
still I myself support @deg-pl - better use attr_* - much more clear and human readable. at_ can easily be confused with references using the english word "at".
There was a problem hiding this comment.
A simple find&replace should do the trick, while it's fresh
|
|
||
| // attribute name = attribute ID | ||
| /** Access or parking fee */ | ||
| const FEE = 2; |
There was a problem hiding this comment.
It violates PSR-12:
Visibility MUST be declared on all constants if your project PHP minimum version supports constant visibilities (PHP 7.1 or later).
Should be:
public const FEE = 2;
| /** Climbing gear requried */ | ||
| const RAPPELING = 3; | ||
| /** Boat required */ | ||
| const BOAT = 4; |
There was a problem hiding this comment.
Napiszę po polsku, bo boję się, że mój angielski wprowadzi tu zamieszanie ;)
Zdecydowanie mam wrażenie, że lepszym wyjściem jest wykorzystanie wprowadzonej przez PHP 7 możliwości definiowania stałych jako tablic. W zaproponowanej przez Ciebie formie już teraz definicja jednego atrybutu jest w minimum 3 miejscach, a kod może się rozrosnąć i będzie jeszcze trudniejszy do serwisowania. Proponuję taką konstrukcję:
public const BOAT = [ 'id' => 4, 'translation' => 'at_boat', 'icon' => 'at_boat', ];
Jeśli chcesz dodać nowy atrybut - robisz to tylko w jednym miejscu, wszystko jest czytelne, nie zostają śmieci po usuniętych atrybutach (wyobrażasz sobie za 10 lat przeglądanie wszystkich Twoich case'ów czy nie ma w nich atrybutów-widmo?
Takie rozwiązanie dodatkowo trywializuje getIconFileName() i getTrKey() - to mogą być krótkie, proste metody, zamiast długich tasiemców.
There was a problem hiding this comment.
hmm.. no ciekawe - nie wiedziałem, że tak już można w PHP - muszę się przyjrzeć...
There was a problem hiding this comment.
OK, przemyślałem i jednak bym został z tym co jest.
Ogólnie to to by było super gdyby dało się definiować "stałe obiekty" - tj mieć jakiś rodzaj interfejsu czy klasy i definiować to inline np tak:
class Attrib {
public $id;
public $trKey;
__construct Attrib($id, $trKey) (...)
}
class Attribs {
public const FEE = new (1,'at_fee');
...
}
ale to na razie nie możliwe...
Za to problem z definicją jako tablica jest to, że nie można wtedy uciec od tego indeksu
i musisz żeby dostać id pisać:
$supportedAttribsIds = [CacheAtt::FEE['id']]
a mi się to mało podoba... te jawne indeksy jakieś takie mało sexy...
Za to te moje switch-case'y są dość stabilne, bo:
- jeśli dodasz atrybut a nie dodasz go do switch-case w którejś z metod - wpadniesz w default i widzisz error w logu
- jeśli usuniesz atrybut a nie usuniesz w switch-case - masz błąd od razu bo php wychwyci to
- zgadzam się, że te długie listy też są mało fajne, ale na razie nie mam prostszego pomysłu - można to polaczyc w jedną tablicę w stylu:
const ATTR_CONF = [
self::FEE => ['trKey' => 'at_fee', 'icon' => 'at_fee'],
...
]
ale nie wiem czy to coś zmienia... No może mamy to w jednym miejscu... OK..
There was a problem hiding this comment.
Alternatywnie można byłoby wczytywać te stałe w postaci tablic i przerabiać je pętlą for na obiekty, ale to chyba byłby przerost formy nad treścią.
Twój kod
$supportedAttribsIds = [CacheAtt::FEE['id']]
można też ewentualnie ugryźć czymś w rodzaju gettera i odwoływać się w stylu:
$attribId = CacheAttribute::getId(CacheAttribute::FEE)
ale to chyba też przerost formy. Taka forma pseudogettera ma tą zaletę, że ukrywa logikę przed pozostałym kodem (hermetyzacja) i docelowo czymkolwiek będzie CacheAttribute:FEE - zawsze będzie działało poprawnie i ewentualny refactoring kodu będzie dotyczył tylko jednego miejsca.
Jako ciekawostkę dorzucę, że w PHP8 kod typu:
class Attrib {
public int $id;
public string $trKey;
__construct Attrib($id, $trKey)
{
$this->id = $id;
$this->trKey = $trKey;
}
}
można skrócić do:
class Attrib {
__construct Attrib(public int $id, public string $trKey)
{}
}
| */ | ||
| public static function getTrKey(int $attr): string | ||
| { | ||
| switch ($attr) { |
There was a problem hiding this comment.
Informational:
In PHP8 there is wonderfull instruction "match". For example:
return match ($attr) {
self:FEE => 'at_fee'
(...)
There was a problem hiding this comment.
It's interesting, but now we had php-7.4 on board...
There was a problem hiding this comment.
please let's not rush to PHP8 yet
There was a problem hiding this comment.
@andrixnet this was only for informational purpose. We don't plan to change PHP version currently.
| */ | ||
| public static function getSupportedAttributes(): array | ||
| { | ||
| return self::getKeyFromGeoCacheConfig('reactivationRulesPredefinedOpts'); |
There was a problem hiding this comment.
"reactivationRulesPredefinedOpts"? I think it is a mistake.
kojoty
force-pushed
the
pju-attributes
branch
from
4341c00 to
655aef5
Compare
|
OK I deliver the current version of the code and we will work on this. |
|
BTW. Please see https://opencaching.pl/test/attributes for debug current config debug purpose :) |
There is no Aircraft nor AskOwner, 2 caches with attrib. OTHER. |
|
no php8 to w ogole wiele nowych uproszczeń w składni - no ale jeszcze
chwilę poczekajmy...
…On Tue, Oct 19, 2021 at 11:15 AM Grzegorz Deja - deg < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/Models/GeoCache/CacheAttribute.php
<#2304 (comment)>
:
> + * Each node support only subset of attributes presents below.
+ *
+ * More details about OC attributes: https://wiki.opencaching.eu/index.php?title=Cache_attributes
+ *
+ * This class doesn't have connection to DB.
+ */
+class CacheAttribute
+{
+
+ // attribute name = attribute ID
+ /** Access or parking fee */
+ const FEE = 2;
+ /** Climbing gear requried */
+ const RAPPELING = 3;
+ /** Boat required */
+ const BOAT = 4;
Alternatywnie można byłoby wczytywać te stałe w postaci tablic i
przerabiać je pętlą for na obiekty, ale to chyba byłby przerost formy nad
treścią.
Twój kod
$supportedAttribsIds = [CacheAtt::FEE['id']]
można też ewentualnie ugryźć czymś w rodzaju gettera i odwoływać się w
stylu:
$attribId = CacheAttribute::getId(CacheAttribute::FEE)
ale to chyba też przerost formy. Taka forma pseudogettera ma tą zaletę, że
ukrywa logikę przed pozostałym kodem (hermetyzacja) i docelowo czymkolwiek
będzie CacheAttribute:FEE - zawsze będzie działało poprawnie i ewentualny
refactoring kodu będzie dotyczył tylko jednego miejsca.
Jako ciekawostkę dorzucę, że w PHP8 kod typu:
class Attrib {
public int $id;
public string $trKey;
__construct Attrib($id, $trKey)
{
$this->id = $id;
$this->trKey = $trKey;
}
}
można skrócić do:
class Attrib {
__construct Attrib(public int $id, public string $trKey)
{}
}
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2304 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAWTAKB4QFSIFGPSJ7EN7G3UHUZJNANCNFSM5GFCBDWA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
HI,
This is general refactoring of attributes for geocaches.
I would like to clean it up, unify, move definition of attributes (config) from DB to files etc.
This is ONLY preparation - after merging this everything works the same - this code is not used at all.
How it works:
https://<node>/test/attributeslike: https://opencaching.pl/test/attributesStill to do:
we need to resolve a few details related to attributes not listed at: https://wiki.opencaching.eu/index.php?title=Cache_attributes or these I don't understand:
we should also update OKAPI code to not use DB for attributes dictionary - I will review the OKAPI needs later
I'm going to merge it soon, as this not affects the working code.
If we decide it's OK I will deliver the changes to use the new approach in views with attributes.
Next after delivering OKAPI changes we will be able to finally remove cache_attrib table from DB :)