opencaching / okapi

A common API for all National Opencaching.XX sites
22 stars 19 forks source link

Refactoring attributes #591

Closed andrixnet closed 4 years ago

andrixnet commented 5 years ago

In reference to https://github.com/opencaching/opencaching-pl/issues/1251

attribute-definitions.xml has be substantially enhanced and synchronized with proper attribute IDs according to the changes to be implemented by https://github.com/opencaching/opencaching-pl/issues/1251 and missing attributes added.

This work is only on attribute IDs, strings not (yet) changed (including translations). Only english strings to added attributes.

Please review these changes to ensure correctness overall.

Changes according to attributes documented here: https://wiki.opencaching.eu/index.php?title=Cache_attributes

Known possible side effects:

Please schedule merging somtime between June 24 and 25th. It would be best to coordinate with corresponding merge at opencaching-pl.

andrixnet commented 5 years ago

In the light of recent conversations, please don't commit yet. Reconsidering parts of the numbering scheme.

following5 commented 5 years ago

It may be best if someone else from OCPL team takes over OKAPI maintenance. I have little time these days for OC, and therefore maybe I am not as open-minded and supportive as I should be for ongoing innovations at OCPL.

andrixnet commented 5 years ago

Considering all that is said here and given the fact that OCDE GPX vs OKAPI GPX is unaffected by this, please reconsider and allow OCPL to move forward. I do not know (at this time) if/when someone from OCPL might take over maintaining OKAPI. AFAIK wrygiel has left for some time now; my own knowledge of PHP is far too inadequate for that purpose anyway.

andrixnet commented 5 years ago

Getting ready to merge at opencaching-pl: https://github.com/opencaching/opencaching-pl/pull/2069 Please schedule a merge at OKAPI soon after.

Thank you.

kojoty commented 4 years ago

@following5, hi are you there? I need your opinion about this issue. Thanks!

following5 commented 4 years ago

Sorry for my long silence.

I have reviewed this change again and can confirm, that it

The latter one means that if developers are using the gc_ocde:attrs option of services/caches/formatters/gpx and rely on the IDs which are currently returned, the OCPL-specific attributes will become "invisible" for these apps until developers implement an update and users install that update.

So far OKAPI has promised developers to always stay backward compatible. This would be the first case when we deliberately break backward compatibility. But as this feature has been enabled for OCPL sites just one year ago and is rather "exotic", it may not be in use yet in any applications. Anyway, though I am not comfortable with this change (and think it is unnecessary), I will leave the decision to implement this up to OCPL developers.

I did not review the changed and new OCPL attribute definitions, due to a lack of time. If you think they are ok, please feel free to merge this PR.

Note that at least the following changes need to be done at OCPL sites as synchronously as possible, so that OKAPI and OCPL website attributes continue to work properly:

Also, the attribute_ IDs in lib/languages/*.php need to be updated for all languages and uploaded to Crowdin (otherwise all the attribute translations at Crowdin would get lost). These translations are not in use yet, but are needed to eliminate the cache_attrib table.

The renumbering of IDs is much work, it is complicated to deploy, may introduce new bugs and will not bring benefits to users or developers. It is a purely cosmetical change. Therefore I do not think this is a good idea. But again, if OCPL developers want to do it, it's your decision.

following5 commented 4 years ago

These translations are not in use yet

or at least were not in use when I defined them some time ago. Don't know what happened since then in OCPL code.

wrygiel commented 4 years ago

I didn't read the discussion here, nor reviewed the changes, but I share @following5's doubts. Be sure to backup all databases before this change, because there's a significant chance that this backfires.