[5.0] Rework session/application DIC to avoid circular dependencies#36499
[5.0] Rework session/application DIC to avoid circular dependencies#36499HLeithner merged 8 commits intojoomla:5.0-devfrom
Conversation
3a734a7 to
54633db
Compare
|
What is a DIC? I hope it’s not a typo and the K is missing 😄 |
|
Dependency Injection Container |
|
Ah, thanks. |
|
@wilsonge API tests are failing, and it could be related to your changes because it complains about something with the input filter: https://ci.joomla.org/joomla/joomla-cms/49850/1/28 |
|
Yup that would be me doing stupid things. |
This is a semi-major rework of the application and session packages to avoid the circular dependencies between the two. As an unideal compromise the request object (i.e. JInput) is created as an explicit resource into the container as it's the main common dependency the two rely on. Test all 5 main applications in Joomla - Console, Site, Admin, API and Installation. Ensure that they are available and work properly. Ensure that session works, by logging in and anything else that uses the session heavily (e.g. mod_logged in the backend). Obviously with this being a change to two of the lowest level pieces of the stack it's hard to predict how it would manifest beyond the major breaks (exceptions etc). So I'm somewhat relying on people testing carefully! None
01d7337 to
f9cd69a
Compare
|
This pull requests has been automatically converted to the PSR-12 coding standard. |
|
@HLeithner please can you review this and merge so it's in early in the 4.3 lifecycle. the sooner it's in the better given the b/c impact should be 0 as this all happens before even system plugins are booted |
|
Is there any possible b/c break if you have your own applicaton? |
|
@wilsonge is this needed for 4.4? I think we are better safe with 5.0, since it's released at the same time it's only a matter of will it break with later php versions. if ok please rebase on 5.0 then I add it to my personal tasks list |
|
I really don't mind where you merge it. But I'm not rebasing this for a 4th minor version in a row without some assurances that it's going to get at least tested. Because it's just a waste of my time. |
|
If I see it right then this shouldn't be a b/c issue, in this case I would like to merge it in 5.0 |
|
can you write a migration guide / what have changed for http://pr-28.manual.joomlacode.org/migrations/44-50/removed-backward-incompatibility against the 5.0 branch please? Then I can merge this and 3rd party could adapter there code if needed |
|
See linked PR |
|
Thanks, I merge this for the next Alpha to get more feedback. |
|
FWIW, even though George calls this a non ideal compromise, it's actually how I have done things in AWF for the same reason. It also makes testing far easier since we can mock the input without having to mock the entire application. So, yay, thank you for doing this! |
Simple PR to correct some comments and copyright date from @wilsonge joomla#36499
Summary of Changes
This is a semi-major rework of the application and session packages to avoid the circular dependencies between the two. As an unideal compromise the request object (i.e. JInput) is created as an explicit resource into the container as it's the main common dependency the two rely on. It's not very architectural best practice - but it's how symfony worked until very recently and it's also how laravel still works - so it's not super terrible compared to global statics :)
Testing Instructions
Test all 5 main applications in Joomla - Console, Site, Admin, API and Installation. Ensure that they are available and work properly. Ensure that session works, by logging in and anything else that uses the session heavily (e.g. mod_logged in the backend).
Obviously with this being a change to two of the lowest level pieces of the stack it's hard to predict how it would manifest beyond the major breaks (exceptions etc). So I'm somewhat relying on people testing carefully!
Someone who understands the finesee's of cookies better than me - I'd also appreciate a review of the cookiePath parts of JoomlaStorage - as before sometimes the cookiePath was an empty string and sometimes '/' and i'm not 100% sure if there's a difference or not. There might be a difference in the administrator directory - depending on exactly the path is determined? (PHP Reference for those brave enough) https://www.php.net/manual/en/function.setcookie.php
Documentation Changes Required
None