? Pending

User tests: Successful: Unsuccessful:

avatar wilsonge
wilsonge
3 Jun 2017

Summary of Changes

Fixes the namespace plugin from @yvesh so it generates the libraries/autoload_psr4.php file correctly. It does not swap to using this file YET.

Testing Instructions

The autoload_psr4 file is now generated without causing unit test fails. The plugin is installed with the installer. When unpublishing an extension, that extension is removed from the mapping.

Documentation Changes Required

None

Additional Notes

Currently postgres doesn't add the namespaces in the SQL file. I'm deliberately not 'fixing' this for now because it causes nightmare conflicts whenever we fix plugin parameters (and it's already a total nightmare in the mysql file). This will have to happen sooner rather than later but I'm leaving it as long as I can get away with it

avatar wilsonge wilsonge - open - 3 Jun 2017
avatar wilsonge wilsonge - change - 3 Jun 2017
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 3 Jun 2017
Category SQL Installation Postgresql Libraries Front End Plugins
avatar wilsonge wilsonge - change - 3 Jun 2017
The description was changed
avatar wilsonge wilsonge - edited - 3 Jun 2017
avatar wilsonge wilsonge - change - 3 Jun 2017
Labels Added: ?
avatar wilsonge wilsonge - change - 3 Jun 2017
The description was changed
avatar wilsonge wilsonge - edited - 3 Jun 2017
avatar Bakual
Bakual - comment - 3 Jun 2017

Why a plugin? Why not integrate it into the installer itself?
Imho there is no reason why something as elementary as this should be handled by a plugin.

avatar wilsonge
wilsonge - comment - 3 Jun 2017

Because the installer doesn't handle extension publish/unpublish (which should also remove the autoload)

Also that means you have to call the installer on every page load to register the listener

avatar Bakual
Bakual - comment - 3 Jun 2017

Because the installer doesn't handle extension publish/unpublish (which should also remove the autoload)

Should it really? We don't remove class aliases and other stuff for unpublished core extensions as well. I don't see why it should be removed from that list in that case.

Also that means you have to call the installer on every page load to register the listener

Huh? I don't understand why you would need a call on each page load. Write the file when an extension is installed/updated/uninstalled and be done.

avatar mbabker
mbabker - comment - 3 Jun 2017

You have to account for a scenario where the file doesn't exist when the
app is started. That's part of what this approach helps with. Otherwise
these autoload paths can't be relied on and we shouldn't do it at all.

On Sat, Jun 3, 2017 at 9:49 PM Thomas Hunziker notifications@github.com
wrote:

Because the installer doesn't handle extension publish/unpublish (which
should also remove the autoload)

Should it really? We don't remove class aliases and other stuff for
unpublished core extensions as well. I don't see why it should be removed
from that list in that case.

Also that means you have to call the installer on every page load to
register the listener

Huh? I don't understand why you would need a call on each page load. Write
the file when an extension is installed/updated/uninstalled and be done.


You are receiving this because your review was requested.

Reply to this email directly, view it on GitHub
#16490 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfoXpxejtwMV0WPVBffTxcAbhoc2hOks5sAbjPgaJpZM4NvJQa
.

--

  • Michael Please pardon any errors, this message was sent from my iPhone.
avatar wilsonge
wilsonge - comment - 3 Jun 2017

Should it really? We don't remove class aliases and other stuff for unpublished core extensions as well. I don't see why it should be removed from that list in that case.

The only class alias' for extensions we have is fields. And I'm not sure what the results of unpublishing that is. But I'm sure it's probably not good. Things like contacts and newsfeeds on the other hand is viable and does happen.

avatar Bakual
Bakual - comment - 3 Jun 2017

You have to account for a scenario where the file doesn't exist when the app is started.

That wouldn't be the first file_exist check we do during an app loading. There are other, even simpler ways of ensuring that file exists.

The only class alias' for extensions we have is fields. And I'm not sure what the results of unpublishing that is. But I'm sure it's probably not good.

Lets assume my extension uses a namespace and relies on the autoloading through this file. My extension has several plugins and modules which interact with classes from the component. Like a helper or a model. Pretty normal.
Now a user disables the component for whatever reason. Can you imagine what happens when you remove the autoloading? The whole site will blow up because those modules and plugins don't find the classes anymore. That doesn't sound like a good idea.
It's one thing when the user uninstalls the component and leaves plugins and modules enabled. Then that error is expected. But when disabling a component the site shouldn't blow up.

I don't understand why you even want to remove the autoloading in that case. What's the gain?

avatar mbabker
mbabker - comment - 3 Jun 2017

That wouldn't be the first file_exist check we do during an app loading. There are other, even simpler ways of ensuring that file exists.

It's not ensuring the file exists. If it is an optional file, then it cannot be relied on in the system in general, everything has to account for that scenario. So it's not just a simple file exists check, it is regenerating the file if needed to ensure the system can run.

avatar nikosdion
nikosdion - comment - 4 Jun 2017

I am writing here my comments on the principle of unpublishing extensions per @wilsonge request last night at JaB. Disclaimer: I have not read the conversation, I got an executive summary from George.

When an extension is unpublished I have no reasonable expectation that it continues working. In fact, I have the reasonable expectation that it stops working since that's the entire point of unpublishing. This applies on linked extensions as well.

Example. When you unpublish the Akeeba Release System component its content plugins should no longer work, no matter if they are published or not. In fact I am already doing this check.

For this reason I strongly believe that once a component gets unpublished its autoloader prefix registrations should be removed.

avatar mbabker mbabker - change - 4 Jun 2017
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2017-06-04 08:48:53
Closed_By mbabker
avatar mbabker mbabker - close - 4 Jun 2017
avatar mbabker mbabker - merge - 4 Jun 2017
avatar Bakual
Bakual - comment - 4 Jun 2017

Disclaimer: I have not read the conversation, I got an executive summary from George.

Maybe you should

When an extension is unpublished I have no reasonable expectation that it continues working.

They don't just work. They will create fatal errors. Which is not really a good user experience.

Example. When you unpublish the Akeeba Release System component its content plugins should no longer work, no matter if they are published or not. In fact I am already doing this check.

Yep, that's best practice and JComponentHelper::isEnabled() is our API to check that. This allows to safely return without creating any errors.
However if your class extends a class from the disabled extension (be that a component or a library) then you can't do that. You either have to load the class yourself or it will create a fatal error.

For this reason I strongly believe that once a component gets unpublished its autoloader prefix registrations should be removed.

Is it the purpose of an autoloader to create a cheap "access control" to classes? We're mixing things here. The job of an autoloader is to allow extensions to autoload their classes. If they should be able to be executed or not is not the job of an autoloader.

And again something gets merged that doesn't work yet at all and has several flaws ?

avatar nikosdion
nikosdion - comment - 4 Jun 2017

@Bakual I was explicitly asked by George Wilson to come here and write exactly what I told him yesterday. Doing a personal attack is not helping in any way. More so when you are blatantly wrong.

I have the (mis)fortune of talking to plenty of real world users of Joomla!. This is not an attack, it is a statement of a fact. I know that when they unpublished the Akeeba Release System component (or DocImport, or Akeeba Subs, or ATS) they expected that the "extension" no longer "loads on the site" as they made sure to very loudly and strongly suggest to me (usually prefixing their feedback with "your f@$%ing software broke my site"). In user-speak this means "the component and all of its associated extensions are no longer participating in the content generation when I disable / uninstall the component". Remember, they are users, not developers. They know they installed the Akeeba Release System "component" - even though it's a package with a component and several plugins and modules.

Then you should consider why people unpublish components. Because they suspect there is a conflict with something else. For example, com_subway is loading the Metro Station v.0.3 library whereas com_cityplanner tries to load Metro Station v.0.4 leading to a break. The user read that com_subway may conflict with com_cityplanner. So they try to unpublished com_subway to see if that works. They assume that if they unpublish com_subway the Content - Subway Link content plugin and the Subway Map module will cease working, i.e. it will no longer try to load their code (or Metro Station v.0.3) or throw a 500 error. If they keep getting a break they will assume that either Joomla! is broken or the developer is an idiot (the latter is true and I have been that idiot in the past because, like you, I wasn't thinking this from the user's perspective).

That is to say: you MUST add a check in your plugins and modules to see if the extension they depend on is installed and published. It is YOUR responsibility against your users. Right now we have to do the kludge of JComponentHelper. With the code that's merged I can simply check if a class exists. This is exactly how it should be. JComponentHelper is a relic of the past.

Please spend more time talking to real world Joomla! users and think about their real world expectations. We write software for our users, not for ourselves.

avatar Bakual
Bakual - comment - 4 Jun 2017

Doing a personal attack is not helping in any way.

Umm? I haven't attacked anyone anymore here.

The expectations of users vary much from one to another. But most of them don't expect a site to fatal crash just because they disabled an extension. That I'm sure.

Anyway, if you all think using a plugin to do such an integral part of the system is fine (which was my initial complaint here) then go that route.
Personally I will make sure the namespace of my extension(s) is registered before I use it. It's a simple line of code which will not harm performance at all. This single line will prevent fatal errors when my component got (accidentally or not) unpublished. Maybe I'm an idiot then (I can live with that fine), but my users will not be angry about me about that, that I'm sure.

Add a Comment

Login with GitHub to post a comment