? Pending

User tests: Successful: Unsuccessful:

avatar wilsonge
wilsonge
21 Aug 2022

Pull Request for Issue #38518 and #38524 .

I didn't like the fix that was merged. But didn't have time to do anything other than moan in the maintainers internal chat during the week. Finally had some time to sit down and test out options during the weekend. This is a likely better architectural piece of code (as we're not going to setup custom super globals - the application controls it all) - but comes with more complexity. I don't believe it breaks any b/c (if it's going to be anywhere it's around me having to slightly move where we set the app root - it has to be outside the constructor else we hit a circular loop - but it's before any extensions are loaded so I think it's fine) but definitely needs some serious testing.

Summary of Changes

Injects the main request Uri from the application (where we already had the duplicate code)

Testing Instructions

Check all 5 applications (console/old style cli/administrator/site/api) work as before patch. Check URL is correctly deduced and the original issue within #38518 is still fixed after applying this patch.

Documentation Changes Required

None?

//cc @nikosdion @HLeithner @roland-d @fancyFranci for careful review

avatar wilsonge wilsonge - open - 21 Aug 2022
avatar wilsonge wilsonge - change - 21 Aug 2022
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 21 Aug 2022
Category Administration CLI Installation Libraries
avatar wilsonge wilsonge - change - 21 Aug 2022
The description was changed
avatar wilsonge wilsonge - edited - 21 Aug 2022
avatar wilsonge wilsonge - change - 21 Aug 2022
The description was changed
avatar wilsonge wilsonge - edited - 21 Aug 2022
avatar wilsonge wilsonge - change - 21 Aug 2022
Labels Added: ?
avatar HLeithner
HLeithner - comment - 21 Aug 2022

Moving from the maintainers chat t here.

Why do you introduce a new "string" in the container?
I think also you shouldn't used deprecated aliases (JApplicationAdministrator)

Shouldn't it be ApplicationInterface::class for the alias?
If you don't want to require joomla/application in JUri you can take the class name as string "Joomla\Application\ApplicationInterface"

avatar wilsonge
wilsonge - comment - 21 Aug 2022

Why do you introduce a new "string" in the container?

I need something to mark the application currently handling the request. It's not a class or interface marker. I don't care if we're rendering the frontend of the site - i'm still in console mode.

I think also you shouldn't used deprecated aliases (JApplicationAdministrator)

Sadly it's not deprecated. It's the main name and you can't create an alias of an alias so it's a b/c break to even change that over :( https://github.com/joomla/joomla-cms/blob/4.2-dev/libraries/src/Service/Provider/Application.php#L72

Shouldn't it be ApplicationInterface::class for the alias?
If you don't want to require joomla/application in JUri you can take the class name as string "Joomla\Application\ApplicationInterface"

This kinda crosses over with the intent by your first question.

It works both ways with the interface. Technically all the applications implement that interface and you wouldn't be able to boot multiple apps at once. Doing it this way where it's a marker for the primary execution application would work but is kinda wrong at the same time?

avatar HLeithner
HLeithner - comment - 21 Aug 2022

Why do you introduce a new "string" in the container?

I need something to mark the application currently handling the request. It's not a class or interface marker. I don't care if we're rendering the frontend of the site - i'm still in console mode.

I think also you shouldn't used deprecated aliases (JApplicationAdministrator)

Sadly it's not deprecated. It's the main name and you can't create an alias of an alias so it's a b/c break to even change that over :( https://github.com/joomla/joomla-cms/blob/4.2-dev/libraries/src/Service/Provider/Application.php#L72

Does it matter in the end if it's an alias or the real name? can we change this in 5.0 without having a b/c nightmare?

Shouldn't it be ApplicationInterface::class for the alias?
If you don't want to require joomla/application in JUri you can take the class name as string "Joomla\Application\ApplicationInterface"

This kinda crosses over with the intent by your first question.

It works both ways with the interface. Technically all the applications implement that interface and you wouldn't be able to boot multiple apps at once. Doing it this way where it's a marker for the primary execution application would work but is kinda wrong at the same time?

Can we load more then one application in the same container? does this makes sense at all? I mean maybe yes but in this case you would init it and wouldn't alias it as application.active in the container right? So you can use the ApplicationInterface::class instead. The same way as it's done with the session.

Not setting the $_SERVER variables is maybe more clean but also more inconvenient for 3rd party code which maybe not using the cms/framework at all. Not sure if this is good or bad.

avatar wilsonge
wilsonge - comment - 21 Aug 2022

Does it matter in the end if it's an alias or the real name? can we change this in 5.0 without having a b/c nightmare?

Alias's can't be aliased in the current code - I tried it. So yes it matters. You can change my alias. It's the service provider that's the problem - but that already exists. So your kinda screwed anyhow from 4.0.0

Not setting the $_SERVER variables is maybe more clean but also more inconvenient for 3rd party code which maybe not using the cms/framework at all. Not sure if this is good or bad.

This is a CMS library. If people are attaching 3rd party libraries that are using Uri (i.e. the CMS) then I don't see it as a big deal. Most 3rd party libraries won't be aware of the CMS at all - but will be aware of console apps. Or they will be aware of the CMS - but then this isn't an issue anyhow

Can we load more then one application in the same container? does this makes sense at all?

Yes - that's the entire point of this code. The console application is loading the site application when booting up MVCFactory (read the original issue)

but in this case you would init it and wouldn't alias it as application.active in the container right?

Correct and that's exactly what I want to happen. That's why I've done it this way.

avatar laoneo
laoneo - comment - 22 Aug 2022

Why not having an URI factory which delivers uris on demand? So we can also get rid of getInstance in the Uri class and the factory can act accordingly to the environment.

avatar nikosdion
nikosdion - comment - 22 Aug 2022

There are good things in this code but there are a few things I don't like, especially if want to maintain b/c and even more so if we want Joomla to move towards a future where the only point of truth will be the global main instance of a dependency injection container.

First of all, you are introducing a string key in the container which did not exist in 4.0.0–4.2.0 inclusive. What it does is basically a more convoluted way of doing Factory::getApplication() with the following difference.

While Factory::getApplication() works even in old CLI scripts extending from CLIApplication your new thing does not as they are either not aware of the container at all (legacy J3 code) or even if they are they don't know about the new key you just introduced. These applications would fall through after issuing deprecated warnings.

Here's the problem. Many people run CLI script with full error reporting. They would see the deprecated notice and complain that the CLI script is broken. As a 3PD I can't do much about it. Even if I am aware of the change I'd have to add an if-block to see if I am running in Joomla > 4.2.0, get the container, set the magic container key and HOPE that you are not going to introduce any other change which expects the application object returned from that key to implement CMSApplication.

Considering that most Joomla 3 sites are still not migrated to Joomla 4 yet and they won't be anytime soon because of the mixed messages sent by the project, 3PDs really need to prioritise Joomla 3 compatibility. Adding something which gets in their way is not a very nice thing to do, especially since the solution is using Factory::getApplication in that code and reworking how Factory::getApplication and application instantiation in general works in Joomla.

Let me be clear. What you did is a good start and is more in line with what I wanted to do but didn't have the time to do. However, it's not deep enough. This is part of what I wanted to discuss with @nibra, @HLeithner and @laoneo tomorrow at 1pm UTC. If you're not working please do join us, I know you're one of the few people who understand the architecture deeply enough. We'll be on Skype. My handle is live:sledge81.

Okay, let's talk about architecture.

Right now, the last three lines of the app.php boot–up script of every application looks a lot like this:

// Instantiate the application.
$app = $container->get(\Joomla\CMS\Application\SiteApplication::class);

// Set the application as global app
\Joomla\CMS\Factory::$application = $app;

// Execute the application.
$app->execute();

I am ambivalent about having a service provider per application type and no centralised Singleton factory to get applications by type. My ambivalence stems from the fact that service providers are anonymous classes which cannot be extended by a developer who wants to construct their application by extending an existing one. We have to duplicate the code in the core. If it changes in the core we're faced with fatal errors. At the same time I can't think of a practical use case where I'd need to extend SiteApplication instead of WebApplication or CMSApplication (but it also does not mean nothing like that exists). I'd strongly prefer having an application factory with adapters so that they can be extended if needed.

The major problem, though, is that the current application object is only known to \Joomla\CMS\Factory. Why are we still doing that? We have a DI container, that should be our sole point of truth. I would propose registering a service provider in app.php which always returns the active application object. I like the key name 'application.active' just fine. Then we can refactor \Joomla\CMS\Factory::getApplication to return the active application from the container.

So, for this PR I would actually remove all of the 'application.active' stuff and only keep three changes with minor modifications:

  1. The implementation of configureBaseUrlForApplication() as–is. I like it and it's not breaking b/c.
  2. The console application doing $this->set('uri.request', $uri->toString()); instead of overwriting the super-globals. This is what I had tried first, I had seen the uri.request being used in WebApplication. However, that code was executed after having already used Uri which made its existence a moot point. I had architectural concerns and time constraints so I went with the cop–out of modifying superglobals. I like your code better, George, it's much cleaner and exactly what I'd want to see!
  3. In the Uri class change $applicationUriRequest = Factory::getContainer()->get('application.active')->get('uri.request'); to $applicationUriRequest = Factory::getApplication()->get('uri.request') for 4.x. Moreover, the trigger_error calls need to change. The one inside the catch should say that there is no current application set in the Factory and the one after it should mention that the current application must provide the 'uri.request' configuration key.

These changes could be implemented in 4.2.2 or another 4.2.x version after testing some older Joomla 3 code. We'd need feedback from 3PDs with real world code! I have my older versions of my software which works in J3 and J4 plus my newer J4 only (and J4 native) extensions that I could test with but I don't think that my software is a representative sample. Also, this would take some time because I am just back from vacation and have to deal with support and new releases, so definitely not something I can do today i.e. before 4.2.1 is released.

I would also say that starting in 4.3.0 we can implement the 'application.active' service provider in core code. In this case we can cater for b/c in the Factory::getApplication method. It would first check self::$application. If it's empty, return the current application from the container. If it's non-empty trigger an E_USER_DEPRECATED notifying the user of the correct way to do things (register application.current in the container, target deprecation is 5.0) and return self::$application. Come Joomla 5.0 this b/c code (and the Factory::$application static public property) can be removed, or at least we can evaluate whether they can be removed. Moreover, at this point we can replace all Factory::getApplication with Factory::getContainer()->get('application.active') and change Factory::getApplication to also do exactly that plus issue a deprecated notice.

By Joomla 6.0 (or 7.0, if we can't drop deprecated code in 5.0) we should only have ONE static method in Joomla\CMS\Factory: getContainer. The DIC should be our sole point of truth.

If we introduce the 'application.active' the way this PR proposes our hands will be tied and I'm afraid it would take too many major versions to fix the problems with the legacy CMS Factory knowing and doing too much to make architectural sense.

avatar nikosdion
nikosdion - comment - 22 Aug 2022

Why not having an URI factory which delivers uris on demand? So we can also get rid of getInstance in the Uri class and the factory can act accordingly to the environment.

A factory returns a new instance of the object every time it's called. Uri needs to return the same instance for an identical URI (it's a Singleton and it MUST remain a Singleton). Changing that would not only be a massive b/c break, it would probably break Joomla itself and definitely break third party extensions.

Moreover, it would not address the issue at hand and make its resolution harder.

There is really no good reason to introduce a URI factory and every reason NOT to do that.

avatar laoneo
laoneo - comment - 22 Aug 2022

Or a registry when you want to return always the same instance. Lets go a step outside, what is the reason to have always the same instance, beside BC? As I used the first time JUri decades ago I was surprised that when I do JUri::root()->setVar(..), that it modifies the global instance and that I had to clone it first.

avatar nikosdion
nikosdion - comment - 22 Aug 2022

I was surprised that when I do JUri::root()->setVar(..), that it modifies the global instance and that I had to clone it first.

That's exactly the reason why it needs to be a Singleton!

Example 1. You've done pagination, right? If you create a comments component which needs to add comments pagination in an article which has its own pagination (page breaks) you need to mess around with the global Uri::getInstance() to make it work.

Example 2. Joomla does not have middleware. Therefore we cannot modify its Request object (which doesn't really exist in the CMS). So how can I modify the request before Joomla does SEF routing and hands execution over to the component? I get the global Uri::getInstance() and modify it, is how. I remember doing something like that in ARS to be able to handle the weird Install from Web URLs.

I could go on and on. We REALLY need this to be a Singleton. We cannot make it non-Singleton until Joomla relies entirely on middleware so this would be 7.0 at the earliest, more like 8.0 or 9.0 if we want to be realistic about how long it can reasonably take core maintainers to come up with a b/c implementation of middleware in Joomla and 3PDs with dozens to over a hundred extensions reinventing the wheel in their extensions to use middleware.

Please remember that Joomla is not a CS assignment, it's a real product used by real people. We can't break userland just because it's “more right” doing things a different way. People —end users, integrators and most 3PDs— don't care if Joomla does things according to CS theory. They care that their sites and extensions work predictably and don't need to be thrown out of the window and rebuilt from scratch every so often. Our job defining architecture is to find out how much we can change without breaking userland, even if what we do is not theoretically perfect. Our target audience is a very long way from the target audience of Laravel, Symfony, Laminas etc. If it makes you sleep better at night, that's the very same attitude Linus Torvalds has towards the Linux kernel development: the primary concern is to not break userland.

avatar laoneo
laoneo - comment - 22 Aug 2022

Nobody wants to break userland but sometimes you have to think a bit out of our Joomla box and see what is possible/doable and if it is still the right way to do, what we actually do. Brainstorming is not a bad thing, it doesn't always mean that it must be done exactly that way, but it can lead to new and better ideas.

avatar nikosdion
nikosdion - comment - 22 Aug 2022

For sure, I fully agree! I always try to think outside of what Joomla currently does and come up with some ideas. I then evaluate them against how things work now and try to see if it's possible to offer b/c and not make people's lives harder.

For example, I was thinking about whether we need some kind of factory service for the application. I am not sure a factory would be a good idea but I also don't like the fact that the service providers for the core application cannot possibly be overridden. We are making an assumption that these applications will HAVE to be constructed a certain way and nobody is to ever change that outside of core code. Is there even a reason for it?

Likewise, trying to document the way Models works I ran into a brick wall with the MVCFactory. You can read about it in https://github.com/nikosdion/joomla_extensions_development/blob/main/joomla_extensions_development.epub If I want to pass custom services to a Model, Controller, View or Table I need to return my own MVCFactory. However, this requires me writing a custom MVCFactory service provider. I cannot extend from Joomla's service provider and Joomla's service provider cannot use my custom MVCFactory class. However, the core MVCFactory service provider DOES change over time which means that if I do things the “right way” (pushing services through MVCFactory) I create a brittle solution which WILL break randomly in a future version of Joomla. Since Joomla treats the service provider as internals it won't even consider it a b/c break, yet my extension WILL be broken because it IS a b/c break. I have to instead pervert the extension class to return the component's DIC so I can pull services into my MVC object when constructing it. This is also bad. This tells me that the way Joomla handles the MVCFactory instantiation is wrong. I am working on a solution which will maintain b/c and fix this problem — exactly because I am thinking outside of the Joomla box and trying to make its DI Container architecture actually useful to 3PDs who most definitely don't use Joomla the way Joomla uses itself.

avatar laoneo
laoneo - comment - 22 Aug 2022

Please open for the MVCFactory service provider problem a new issue, so we can discuss it there to not shift away from the intention of this pr.

avatar nikosdion
nikosdion - comment - 22 Aug 2022

Will do! I just need a quiet 2 hours to ponder on a meaningful solution.

avatar wilsonge
wilsonge - comment - 22 Aug 2022

While Factory::getApplication() works even in old CLI scripts extending from CLIApplication your new thing does not as they are either not aware of the container at all (legacy J3 code) or even if they are they don't know about the new key you just introduced. These applications would fall through after issuing deprecated warnings.

Don't disagree - that's why I mentioned up top the old CLI Apps were a priority for testing.

In my first commit I had this key registered in CliApplication. But reality is said CLI applications won't have the uri.request property set anyhow (unless we also add that in CliApplication in a copy/paste from ConsoleApplication?)

Doesn't https://github.com/joomla/joomla-cms/blob/4.2-dev/libraries/bootstrap.php#L59-L61 stop the deprecated warnings showing in console though (N.B. haven't tested as in work - just was my understanding)

Considering that most Joomla 3 sites are still not migrated to Joomla 4 yet and they won't be anytime soon because of the mixed messages sent by the project, 3PDs really need to prioritise Joomla 3 compatibility

You know I agree with this and it's part of the reason many of the J4 designs were as they were.

I would propose registering a service provider in app.php which always returns the active application object

But how would that service provider know? Plus don't you still need to register it with a string anyhow. There's no class which tracks what the active primary application actually is? Not against it. Just not seeing how it works practically in order to make the change.

In the Uri class change $applicationUriRequest = Factory::getContainer()->get('application.active')->get('uri.request'); to $applicationUriRequest = Factory::getApplication()->get('uri.request') for 4.x. Moreover, the trigger_error calls need to change. The one inside the catch should say that there is no current application set in the Factory and the one after it should mention that the current application must provide the 'uri.request' configuration key.

If we did this change we wouldn't have a catch I guess - because no such thing as a nokeyfoundexception in that case. We can check the global is null (but we'd end up in recursion loops at this level of the cms anyhow I think if it wasn't set).

In this change aren't you still giving old cli apps where uri.request won't be set a deprecated message though - which was the point of your b/c message above?

These changes could be implemented in 4.2.2 or another 4.2.x version after testing some older Joomla 3 code. We'd need feedback from 3PDs with real world code! I have my older versions of my software which works in J3 and J4 plus my newer J4 only (and J4 native) extensions that I could test with but I don't think that my software is a representative sample. Also, this would take some time because I am just back from vacation and have to deal with support and new releases, so definitely not something I can do today i.e. before 4.2.1 is released.

Happy to leave things later - but my concern is about making sure that we don't end up in b/c issues with setting the superglobals in 4.2.1. As long as we're not in b/c issues reverting the superglobals later on then I'm happy.

This is part of what I wanted to discuss with @nibra, @HLeithner and @laoneo tomorrow at 1pm UTC. If you're not working please do join us, I know you're one of the few people who understand the architecture deeply enough. We'll be on Skype

I'll do my best. But yeah that's typically work hours for me - so no guarenteees.

avatar nikosdion
nikosdion - comment - 22 Aug 2022

But reality is said CLI applications won't have the uri.request property set anyhow (unless we also add that in CliApplication in a copy/paste from ConsoleApplication?)

Unless the developer does something along the lines of

$this->set(
  'uri.request',
  ComponentHelper::getParams('com_example')->get('siteurl', 'https://console.invalid')
);

This is far better than hacking CMS\Uri with Reflection to make routing work (the Joomla 3 way of doing things...).

Doesn't https://github.com/joomla/joomla-cms/blob/4.2-dev/libraries/bootstrap.php#L59-L61 stop the deprecated warnings showing in console though (N.B. haven't tested as in work - just was my understanding)

Yes, it does. Usually I get “bug” reports on notices I know about and know why I can't work around yet from people running in Debug mode. Of course it's very common with my software since it's used for in-development sites which might not be the case for most 3PDs...

But how would that service provider know? Plus don't you still need to register it with a string anyhow.

Replace this line

$app = $container->get(\Joomla\CMS\Application\SiteApplication::class);

with

$container->alias('application.current', 'JApplicationSite');
$app = $container->get('application.current');

or, alternatively, do this (I don't like it):

$app = $container->get(\Joomla\CMS\Application\SiteApplication::class);
$container->share(
    'application.current',
    function (\Joomla\DI\Container $container) use ($app) {
        return $app;
    }
);

Eventually, this would all be rewritten as

$container->share(
    'application.current',
    function (\Joomla\DI\Container $container) {
        $container->get(ApplicationFactory::class)->createApplication('site')
    }
);

If we did this change we wouldn't have a catch I guess - because no such thing as a nokeyfoundexception in that case. We can check the global is null (but we'd end up in recursion loops at this level of the cms anyhow I think if it wasn't set).

Sorry, you lost me here.

\Joomla\CMS\Factory::getApplication is a “dumb” method, it only returns self::$application or throws an exception, it does not cause recursion. If the application is not set we catch the exception in the try-catch block.

If the application is set we can check if it implements \Joomla\Application\ConfigurationAwareApplicationInterface. If it doesn't, throw a RuntimeException to be caught by the try-catch.

Then get the 'uri.request' (which, by the way, is meant to be transient configuration, inferring from how it's attempted to be used elsewhere in the CMS). If it's not empty, use it. Otherwise, show a notice.

In this change aren't you still giving old cli apps where uri.request won't be set a deprecated message though - which was the point of your b/c message above?

You would! In most CLI applications you'd see the message that the Factory does not have an application. Okay, you set that, e.g. by changing this code:

\Joomla\CMS\Application\CliApplication::getInstance('ExampleCLI')->execute();

to

\Joomla\CMS\Factory::$application = \Joomla\CMS\Application\CliApplication::getInstance('ExampleCLI');
\Joomla\CMS\Factory::getApplication()->execute();

NOW you get the second message from outside the try-catch block telling you to set the 'uri.request'.

This is developer experience: tell the developer about the issue they need to fix, not about fifty steps ahead of where their code is now. Help them fix one issue at a time.

Happy to leave things later - but my concern is about making sure that we don't end up in b/c issues with setting the superglobals in 4.2.1. As long as we're not in b/c issues reverting the superglobals later on then I'm happy.

I have a confession to make. I've been doing this since Joomla 1.6. Even my current, Joomla 4 only code does this because it was the only way I could get URL routing to work under CLI. No issues reported.

What we SHOULD do is have something like a developer's bulletin documenting bad and unsupported practices such as using directly web accessible .php files instead of com_ajax, putting CSS and JS in the component's folder instead of its media folder and, of course, accessing superglobals instead of going through the application's input object. This is our ‘exit clause’. If your code is doing something unsupported and we make a change which breaks it that's too bad, you had been warned, now you get to keep both pieces. Considering the number of changes already in 4.0 breaking things which have been unsupported since Joomla 1.5 (my examples are far from being randomly selected...) I'd say it's high time we did that.

I'll do my best. But yeah that's typically work hours for me - so no guarenteees.

No worries, I understand. I'd normally propose evening hours but we're in the summer house and I can't take a meeting near my daughter's bed time; I don't have an office here with adequate sound–proofing.

avatar joomla-cms-bot joomla-cms-bot - change - 25 Aug 2022
Category Administration CLI Installation Libraries Installation Libraries
avatar wilsonge
wilsonge - comment - 25 Aug 2022

@nikosdion sorry I couldn't make the meeting yesterday - ended up having to take a trip to London for the day for work. Will chat to Harald and try and get filled in

I've removed container references and added the deprecation messages as you suggested. We can come back to active application objects in 4.3 outside of this PR.

avatar wilsonge wilsonge - change - 25 Aug 2022
Title
Rework how URLs are injected into applications
[4.2] Rework how URLs are injected into applications
avatar wilsonge wilsonge - edited - 25 Aug 2022
avatar wilsonge wilsonge - change - 25 Aug 2022
The description was changed
avatar wilsonge wilsonge - edited - 25 Aug 2022
avatar joomla-cms-bot joomla-cms-bot - change - 26 Aug 2022
Category Installation Libraries External Library Composer Change Installation Libraries
avatar HLeithner
HLeithner - comment - 2 May 2023

This pull request has been automatically rebased to 5.0-dev.

avatar HLeithner
HLeithner - comment - 30 Sep 2023

This pull request has been automatically rebased to 5.1-dev.

avatar HLeithner
HLeithner - comment - 24 Apr 2024

This pull request has been automatically rebased to 5.2-dev.

avatar HLeithner HLeithner - change - 24 Apr 2024
Title
[4.2] Rework how URLs are injected into applications
[5.2] Rework how URLs are injected into applications
avatar HLeithner HLeithner - edited - 24 Apr 2024

Add a Comment

Login with GitHub to post a comment