Skip to content

Comments

ExtensionAdapter should expect a client_id as either integer or string#35822

Closed
nikosdion wants to merge 1 commit intojoomla:4.0-devfrom
nikosdion:fix/35376-update-adapter
Closed

ExtensionAdapter should expect a client_id as either integer or string#35822
nikosdion wants to merge 1 commit intojoomla:4.0-devfrom
nikosdion:fix/35376-update-adapter

Conversation

@nikosdion
Copy link
Contributor

@nikosdion nikosdion commented Oct 14, 2021

No description provided.

Fix the \Joomla\CMS\Updater\Adapter\ExtensionAdapter to provide
backwards compatibility with integer `client_id` values in update XML
manifests (a.k.a. update sites)
* integer key is only provided in the 4.x release as a means to facilitate
* migration of legacy extensions to Joomla 4. Joomla 5 will remove the b/c code and
* require client_id to be a string key. The string key is one of 'site' or
* 'administrator'.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry to be pedantic but couldnt the client key also be api?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, no and I did check 😉 We do not have api–specific extensions. In fact, the only way to extend the API is by adding an <api> target in the XML manifest of a component. Components are installed in the administrator application i.e. their client_id is 1.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok - cool

@joomdonation
Copy link
Contributor

I have tested this item ✅ successfully on 66b3950

Fixed the issue as described.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35822.

@zero-24
Copy link
Contributor

zero-24 commented Oct 14, 2021

For Joomla 1.6 up to and including 3.10 it's an integer: 0 for ‘site’ and 1 for ‘administrator’. For Joomla 4 you should use a string matching the application name, currently site or administrator.

I have not looked up when it got changed but atleast since 9 years (where the B/C layer got added) both where working fine. So saying "up to 3.10 it requires to be an integer" seems to be wrong to me as that was alteast since 9 years a b/c handling showing in the logs.

Thats also the reason it was finally removed and documented to be gone with J4.

When such warnings and notes are ignored for a that long time I guess there is no point in changing or removing this "b/c" handling ever again as it will come back to the table right after any major release where souch a change could get implemented. So I would say go restore it but make it permanent and remove the "deprecated" warning from J3. :(

Ah looks like @HLeithner took the decision about adding a b/c note again: #35376 (comment)

@HLeithner Can I please request you to decide to remove the "deprected" tag and just add a note to make sure it will stay forever for the reasons noted above.

@brianteeman
Copy link
Contributor

This is really frustrating as I have been saying the same thing about isSite and isAdministrator but maintainers used the same argument (we made it deprecated x years ago)

@HLeithner
Copy link
Member

Sorry I trusted @nikosdion on his words

DO NOT change the documentation to make a string again. This will break updates for extensions which are compatible with both Joomla 3 and 4.

Which seems to be wrong because since 2012 joomla logs a deprecation warning....

So now I'm the idiot because I didn't checked the source my self and already accepted this change... can only loose here...

@nikosdion
Copy link
Contributor Author

I'll just put it this way.

OSM runs a directory, JED. This is integrated into OSM's product, Joomla. Not being listed there can be a serious problem for 3PDs.

JED requires 3PDs to use Joomla's extensions updater for their software to be listed. To do that, they need to create an update site for their extensions.

The official documentation of the update site XML structure even at the time of this writing very clearly states that the client/client_id key MUST be an integer.

So, this leaves us with two possibilities:

A. The documentation is right and the code in Joomla 4 is wrong. This is the stance I took because it's the stance that cannot get OSM sued.

B. The documentation is wrong and the code is right. This is the stance @zero-24 and @HLeithner are taking. In this case the official OSM documentation misleads 3PDs into being non–compliant to OSM's requirement for inclusion in the JED. This gets OSM sued and as we have seen from Epic vs Apple this can indeed get you in very hot water.

Stop thinking just like engineers and start thinking about the bigger picture. Do you REALLY want to take a stance that opens OSM to getting sued by anyone who got excluded from the JED because the Joomla extensions updates wouldn't work for them because the documentation was not in line with the code OR do you want to suck it up, pretend the code was wrong all along and fix it in a way that won't get OSM sued?

Anyway, my extensions are not affected because I am doing things by the code, not by the documentation. If you don't want to protect OSM against a very likely sueball I am not going to stand here and be told once again that I don't know what I am doing — I actually understand much more of the bigger picture than you do all together.

I am closing this PR and good luck to OSM.

@zero-24
Copy link
Contributor

zero-24 commented Oct 14, 2021

B. The documentation is wrong and the code is right. This is the stance @zero-24 and @HLeithner are taking.

Just for the record as I have been blamed here:

  • Yes the docs page is wrong and yes it should be corrected.
  • I have not asked for this PR to be closed nor redjected in any terms.
  • The decision to add this check back was based on a wrong claim "This will break updates for extensions which are compatible with both Joomla 3 and 4.".

So in conclusion yes there is an issue here and the options we have are:

  • add the check here back and try it again with J5
  • add the check back and keep it permanent
  • dont add the check back

All of them have pro's and con's for sure :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants