[#36499] - [5.0] Rework session/application DIC to avoid circular dependencies
- Fixed in Code Base
- 12 Jul 2023
- Medium
- Build: 4.1-dev
- # 36499
- Diff
- wilsonge:feature/fix-circular-dependencies
User tests: Successful: Unsuccessful:
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
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Installation Libraries |
| Labels |
Added:
?
|
||
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.
| Title |
|
||||||
| Labels |
Added:
?
Removed: ? |
||
This pull requests has been automatically converted to the PSR-12 coding standard.
| Labels |
Added:
?
|
||
| Title |
|
||||||
@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?
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.
obuisard
- | Title |
|
||||||
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
| Labels |
Added:
Feature
PR-5.0-dev
Removed: ? ? |
||
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
| Status | Pending | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2023-07-12 06:16:44 |
| Closed_By | ⇒ | HLeithner |
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!
What is a DIC? I hope it’s not a typo and the K is missing?