[4.0] Joomla CLI breaks if absolute path is used and System Page Cache plugin is enabled#35877
[4.0] Joomla CLI breaks if absolute path is used and System Page Cache plugin is enabled#35877HLeithner merged 5 commits intojoomla:4.0-devfrom tampe125:feature/fix-cache-cli
Conversation
First of all, it really doesn't make any sense, second a fatal error could happen since we were trying to fetch the URL
|
Yes, this is the right approach to solve the issue. This PR solves #35722 @alikon @ceford @PhilETaylor #35625 @Kubik-Rubik This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35877. |
| */ | ||
| public function onAfterRender() | ||
| { | ||
| // Run only when we're on the public section |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
@anibalsanchez You mean it replaces @alikon 's PR? |
|
I have tested this item ✅ successfully on 8eda35e This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35877. |
|
@richard67 Yes, I mean that it replaces it. |
|
I have tested this item ✅ successfully on 8eda35e This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35877. |
|
RTC This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35877. |
|
@tampe125 As your pull request has 2 good tests, I've set the RTC status, which means ready to commit. But it would be nice if you could check @PhilETaylor 's suggestion about changing the code comments. If you change that, it will not need to be tested again because it's just code comments. Thanks in advance. |
| parent::__construct($subject, $config); | ||
|
|
||
| // Run only when we're on the public section | ||
| if (!$this->app->isClient('site')) |
There was a problem hiding this comment.
shouln't run on isClient('api') ?
|
Ok, I'll update code comments. |
|
One additional note on the issue: the problem's source is the execution of a "Web Only" plugin in the Console context. The proposed conditional to avoid the execution is going to solve the issue. However, the same problem will be found in all the "Web Only" plugins, and more of these conditionals will be required. It could be a better solution in the long term to create an interface "WebApplicationPlugin" and avoid executing these plugins in the dispatcher. |
|
But then people will complain that they need to update their extensions. Can't win. |
|
@brianteeman All of these developments are part of the new official support of the Console Apps. So, they are needed to complete the implementation. In the past, you could hack your way around these problems. For instance, to create a native command based on Joomla. But, they were error-prone, and you always got notices and warnings. |
|
You missed the tongue in cheek comment - never mind |
|
@anibalsanchez I strongly disagree with your proposal. The EventDispatcher is finally and correctly disentangled from the application object. In fact, it is possible to instantiate it outside the scope of an application and dispatch any kind of custom events. What you proposed is a major step backwards as it would make EventDispatcher aware of and entangled with the application object. Moreover, what you propose does not address what happens if another web application is added to Joomla or what happens if a developer implements a custom application e.g. by extending WebApplication or CliApplication (deprecated) / ConsoleApplication. Extending WebApplication to create custom entry point files which fire Joomla's system plugins is something very common with payment processor integrations. There are several payment processors which do not allow query string parameters in the callback URLs, making the use of a component or com_ajax impossible without some serious (and precarious!) .htaccess / web.config / NginX configuration magic. Ultimately, it's the plugin developer's responsibility to consider the context of their plugin and decide which Joomla applications it should run in. A plugin which must only fire in the frontend can use Also, to set the history straight, using an EventDispatcher that's disentangled from the Application object has ABSOLUTELY NOTHING AT ALL to do with the ConsoleApplication or the ApiApplication. I can tell you that as the developer who integrated the Joomla Framework Event package with Joomla 4 back in August 2015, long before either of these applications were created. Using immutable events has all to do with performance and preventing problematic interactions between different plugins handling the same event depending on their order (for example: plugins changing the type of a parameter passed by reference between array and object, breaking plugins further down the event pipeline). Eventually the “classic” plugins will go away and it will no longer be possible to pass parameters by reference in an event being handled by plugins. This will solve all those issues of yesteryear since you can only add to the Event's results while making for better performance because of the superior handling of events in the Events package. |
|
@brianteeman I edited my original comment to only include the reasoning of my disagreement with Anibal. I removed my reply to your reply so you can now delete your reply to my comment as well. |
|
@nikosdion Thanks for the analysis. I agree that the is better if the EventDispatcher knows nothing about the clients. The problem is that all system plugins are auto-subscribed to all events regardless of the application context and they have to check the context and eject the execution. It would be better to look for a way to discriminate the events at the source. |
|
@anibalsanchez You really have no idea how Joomla 4 works. Fair enough, let me elucidate. Let's start backwards, with Joomla 1.0 to 3.10 inclusive. The plugin system in those versions was a clumsy implementation of the Observable/Observer pattern. As a result, every plugin being loaded would immediately have all its public methods registered as observers. This could, indeed, cause problems. You needed to duplicate the code which checks the conditions for whether your Observer should fire in each and every public method — unless you implemented clever hacks like using a flag in the plugin object which is initialised in the constructor, a pattern I have used extensively. In Joomla 4 the legacy plugins do get the same treatment BUT THIS IS NOT THE CANONICAL WAY TO WRITE PLUGINS! As I explained before, Joomla 4 uses the Joomla Framework Event package to handle immutable Events instead of the clumsy implementation of the Observable/Observer pattern of yesteryear. Joomla 4 already has the Furthermore, the public methods in the plugin no longer receive a number of parameters and return whatever random data type. Instead, they receive a Anyway, the things you should take from this is that a PROPERLY IMPLEMENTED, NATIVE, JOOMLA 4 plugin can decide for itself which event handlers to register. It's not FORCED to register everything anymore! Therefore your suggestion is completely useless and violates the architecture of Joomla 4. Further to that, what you propose is simply a terrible idea that will NOT work correctly because we have more applications than the ones already built into Joomla itself. Anyone can extend from WebApplication, CMSApplication, ConsoleApplication or even CliApplication to create their own custom application. I already told you that this does happen with payment processor callback plugins. Having interfaces which only address the core applications omits all custom applications. This means that the system behaviour will be vastly different in those custom applications. I will give you two examples where this can lead to catastrophic failure:
Things can get even more hairy than that. This was just the low hanging fruit based on two things I was providing support for just this morning. The other major architectural problem is that Joomla 4 is designed to allow legacy Joomla 3 extensions to work on Joomla 4 (with minimal modifications) to drive the adoption of Joomla 4. To clarify, the goal is that a Joomla 3 extension can be slightly modified to work on BOTH Joomla 3.10 AND Joomla 4.x with the same package. Developers need to focus on the few things that break between Joomla 3 and 4 instead of reimplementing their entire application from scratch with Joomla 4 core MVC right away. What you propose would completely break this expectation. You would need separate packages for Joomla 3 and 4 since there's not going to be a Joomla 3.11 and you can't arbitrarily add a bunch of interfaces that do sod all in Joomla 3.10, nor can you break Joomla 3.10 by having the interfaces do something. Even worse, you enter a situation where a plugin can EITHER work on Joomla 3/4.0 OR Joomla 4.whatever. This makes upgrading Joomla 3 site and updating Joomla 4 sites impossible. You can't install the new version of the plugins on an older Joomla version and you can't update to the newer Joomla version before installing new versions of the plugins. Essentially, you are asking people to trash their sites and build them afresh. And no, disabling all plugins before an update/upgrade or uninstalling all extensions is not a solution; if you have ever maintained a real world site you know why. So, this immediately becomes a no–go. You could have interfaces to DISABLE execution in certain core applications but why do that when there's the SubscriberInterface which allows you to do that and have even more fine grained control?! It all sounds like a pointless exercise because you are missing a fundamental architecture point of Joomla 4, the migration from Observers to Events in plugins. The correct solution, as I said before, is documenting and communicating this. I would also say that Joomla itself should move its core plugins from the legacy implementation to the modern Event system. I don't know why it's not done yet. The Events package was integrated in August 2016 and the SubscriberInterface was added a few months later, in February 2017. |
|
Thanks for the clarification @nikosdion I didn't know there is now a better way to subscribe to events. |
|
@anibalsanchez Considering that you are definitely not a newbie developer I am worried that Joomla may have not made it very clear that the plugin system has changed into an event subscription system. If you know who would be best to contact about developer documentation please do and let them know I can help with an example plugin if need be. I have no idea who to contact about this :/ |
|
I read the comments by @nikosdion and was wondering how to turn them into useful documentation. By coincide, this morning I was working on the Joomla 4 Tutorials project page where there is a description of Creating a Plugin for Joomla - the history shows it was started by WilsonGE and contributed to by Sandra97 plus others. Is that clear enough? It is here: https://docs.joomla.org/J4.x:Creating_a_Plugin_for_Joomla I would offer to do revision but my level of understanding of the architecture is probably not good enough! |
|
@nikosdion @ceford In my case, when I read the code and the documentation, in theory, it sounds great, but at the end of the day, you find the same errors as before. For instance, the previous ideas to solve this bug are:
What is missing is a simple way to know how to do it right. The available information (and our previous experiences) led us to do it as before. For instance, in this case, it looks like that the modern way to do it right would be something like: public static function getSubscribedEvents(): array
{
if ($this->app->isClient('site')) {
return [
'onAfterRoute' => 'onAfterRoute',
'onAfterRender' => 'onAfterRender',
'onAfterRespond' => 'onAfterRespond',
];
}
return [];
} |
|
@ceford This is kinda right but not quite. The recommended method is a mélange of the Joomla 3 plugin–is–a–class way and the Joomla 4 plugin–is–a–bootable–extension way. I think it was written like that because the installer can still not install a fully native Joomla 4 plugin without doing some workarounds. For the curious, here's what a fully native Joomla 4 plugin looks like and here is the workaround to the Joomla installer bug — yeah, the workaround is an empty legacy plugin class which is never loaded. The reason to use this method is, of course, that you have class autoloading for the plugin which makes a lot of sense in complex plugins, like the one I linked to. The alternative was to use traits upon traits to make the code more readable, something I've done in Joomla 4's plugins/system/webauthn. But I digress. I think the current documentation page is at the very least a good start because it tells people to use the SubscriberInterface. Once a developer gets that right everything else is a matter of very simple refactoring down the line. @anibalsanchez I can tell you my opinion on every method and I then I will tell you what I told Davide when he asked me. Setting HTTP_HOST in CLI: Yes... but no. The reason for that is that Checking the application: Yes, it's the correct approach in the context of the code of this plugin as it stands right now. That's basically what I told Davide when he asked me how it should be fixed. I told him that to go for the minimally viable solution, checking if we are in the correct application context, to minimise the chance of this fix being rejected. Using SubscriberInterface. Sure, that's the best approach — as long as we simplify the if–block to return early when the application type doesn't match. Early returning if–blocks are much easier to read. However, the code you typed is not enough! You need to change the signatures of Here's an example of production code which demonstrates how it works: https://github.com/akeeba/release-system/blob/cbcfb6d8ddd000b8a07e8ba7dec66f1a12ea4f5e/plugins/content/arsdlid/src/Extension/Arsdlid.php#L77-L96 Compare that code with any other onContentPrepare event handler to understand the difference. Instead of public function onContentPrepare(Event $event)
{
[$context, $article, $params, $limitstart] = $event->getArguments();The rest of the body goes on as per usual. Since that code is for an event that does not have a return value it doesn't show how you return a value. If you were handling an event where you need to return $result = $event->getArgument('result', []);
$event->setArgument('result', array_merge(is_array($result) ? $result : [$result], [$yourResult]));
// No other return here; the method is voidAs it becomes immediately obvious, changing the method signatures is not something that you can approve without a second thought. Someone needs to go and do a lot of testing to make sure that when these events fire we don't get any unexpected artefacts. To the best of my knowledge only So, the only acceptable solution for a patch release is what Davide implemented in this PR. It's the best compromise between being practical and pedantic. |
|
In theory, today, Joomla 4.1 Alpha 2 is being released. I run a few tests with @ceford About the Joomla 4 plugin documentation: I think that the page has the bare minimum information. By experience, I know that a plugin must be located in a specific folder, with a specific file name, with a particular class name, with a set of methods, and now I know that on J4, it can use the new About documentation examples with a respectable organization, a couple of links:
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35877. |
|
@anibalsanchez I would encourage you to make a PR against the 4.1-dev branch with the One True Joomla 4 Way. Add me as a code reviewer so I can take a look. For Joomla 4.0 this PR here is correct and perfect, addressing a major issue: you cannot set up CRON jobs on a real world commercial host if the Page Cache plugin is enabled. Since this is not an immediately obvious conclusion let me explain. CRON jobs in commercial hosting (be it cPanel, Plesk, a custom hosting control panel or directly editing the |
|
I have tested this item ✅ successfully on c685df1 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35877. |
|
I am working on converting an example I have to the pure way. Right now I am stuck on the @anibalsanchez - I agree, organisation of the documentation is poor and any attempt to improve it may make it worse. Searching turns up stuff going back to J1.5 so you get a haystack when looking for a needle. I will take a look at the ones you mentioned. Thanks. |
|
Having used the cli package myself for a few a few months I created a tutorial just a week ago. I have now figured out how to use the pure Joomla 4 plugin architecture with a cli command. It seems the example provided by @nikosdion does not work with the cli interface because some of the code needs an AbstractCommand parent rather than a CMSPlugin parent. I just know in my bones that the two-stage solution I have come up with is probably not the best, or even dangerously wrong. If anyone has time to take a look it is here: https://github.com/ceford/j4xdemos-plg-onoffbydate The content of a tutorial on the Joomla 4 Tutorials page features this stuff and I don't want to lead anyone astray. |
|
@ceford Oof, we are getting way off topic here. There are many ways to skin a cat but there's definitely an easier way than yours. Not necessarily better, but far more straightforward for a task as simple as registering a simple CLI command which doesn't depend on any other extension. First, make sure your plugin class (the one extending CMSPlugin) gets an instance to the application: protected $app;(yes, no code required, it's populated by the CMSPlugin constructor automatically, all you need to do is declare the property protected or public). Make sure your plugin implements the SubscriberInterface. Register an event handler for the public static function getSubscribedEvents(): array
{
return [
Joomla\Application\ApplicationEvents\ApplicationEvents::BEFORE_EXECUTE => 'registerCLICommands',
];
}In the registerCliCommands method you can instantiate your command and push it to the Console application: public function registerCLICommands(ApplicationEvent $event)
{
$commandObject = new \Joomla\Plugin\System\Onoffbydate\Console\OnoffbydateCommand();
$this->app->addCommand($commandObject);
}Finally, edit your command to add its name: protected static $defaultName = 'onoffbydate:action';No need to mess with services. Everything about the command is included in a single command class and command registration is included in a single plugin class. You can scale this very easily by having an array of command classes' FQNs (fully qualified names), e.g. $commands = [
\Joomla\Plugin\System\Onoffbydate\Console\OnoffbydateCommand::class,
\Joomla\Plugin\System\Onoffbydate\Console\AnotherCommand::class,
];
foreach ($commands as $fqn)
{
$commandObject = new $fqn();
$this->app->addCommand($commandObject);
}We use this pattern in our (for a fee) software to have a single If you want to hit me up with more questions feel free to go to www.dionysopoulos.me (my blog) and use Contact Me to get in touch. |
|
@nikosdion Thank you. Your suggestions are now incorporated into the plugin and it works. The associated tutorial is here: https://docs.joomla.org/J4_CLI_example_-_Onoffbydate#Check @anibalsanchez I looked at the documents you cited and remembered I have seen them before. My immediate thought is they are not Wikis! But those sorts of layouts are what I would like to see. |
|
I have tested this item ✅ successfully on c685df1 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35877. |
|
@ceford You're welcome! I am glad I could help 😄 Thank you for the test, too! BTW, the reason I have not filed a successful test is that Davide is a coworker and asked me about how to best address this issue. I believe that me filing a successful test on code implementing a solution I verbally described would be a case of “the priest blessing his beard” as we say in my part of the world. |
|
I have tested this item ✅ successfully on c685df1 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35877. |
|
@Kubik-Rubik you suggest a change and at the same time submit a successful test? What shall we do now? Set RTC? Or wait until your suggested change gets some reply? |
|
@richard67 Well, the check is unnecessary here, but it does not interfere with the primary purpose of this pull request. :-) |
|
@Kubik-Rubik So if I set RTC it might get merged with that unnecessary check and it will be forgotten, and if the check is removed we have to remove RTC and test again to be sure it was redundant. That is not very effective. Either a PR is good as it is or it is not. If I have successfully tested and I know there is still something wrong in code I think I should not give a good test. Knowing that the PR possibly added redundant code I don’t feel comfortable to give RTC now. Shall someone else do that. |
|
@richard67 Can't you make the change directly as someone with commit rights to the repo? |
@nikosdion I prefer to leave that to someone else. |
|
I’ve pinged other maintainers so they can decide. |
|
Thanks for the PR and thanks for the explanation @nikosdion sadly the documentation (didn't found a pull request with documentation to the new Event/Subscriber system). Adding this to the documentation would be great. Here in this PR it get lost... |
|
Harald, I think it would be most beneficial to write a tutorial for a modern plugin. It’s on my long list of things I’d like to do — once I have converted my last two extensions to fully native Joomla 4 code. I am nearly done with one, the other one is much smaller. I believe that by the time 4.1 will be released I will be able to write a short tutorial, explaining the what and the why.
|
|
@HLeithner I've submitted PR #35986 for review to organize and help in the plugin improvement and documentation. |
If you have the plugin System - Page Cache enabled and you use Joomla CLI entry point with an absolute path (as it happens when it's triggered by a CRON job) a fatal error would happen.
Summary of Changes
This is a side effect caused by enabling the plugin System - Page Cache. Once enabled, it's always loaded, even in contexts where cache should never be allowed (ie backend, CLI etc etc).
In the constructor it tries to parse the current URL; usually this won't cause any harm, beside a warning:
However, everything breaks if an absolute path is supplied:
This happens because the absolute path is "converted" into a URL
http:////var/www/html/joomla/cli/joomla.php(note the 3 slashes) and then it fails to be parsed.The proper solution is to let this plugin run only when it makes sense, in the public section of the website.
Testing Instructions
1 - Enable the System - Page Cache plugin
2 - Open a terminal, navigate to the cli folder and type
php joomla.php list3 - Only a notice should happen
4 - Now use the full path
php /path/to/cli/joomla.php listActual result BEFORE applying this Pull Request
A fatal error should happen
Expected result AFTER applying this Pull Request
Everything should work as expected
Documentation Changes Required
None