? Success

User tests: Successful: Unsuccessful:

avatar mbabker
mbabker
4 Oct 2015

This pull request adds "poor man's" support for dependency injection into the JMenu class tree by utilizing the existing options array to allow class objects to be injected. This helps to identify the class chain's dependencies on other class objects, remove hard couplings to JFactory, and improve testability of the class chain. Also included in this pull request is minor internal structure cleanup and improvements to a couple existing doc blocks.

Test Instructions

With the patch applied, a user should be able to navigate the application (site and admin) without issue.

avatar mbabker mbabker - open - 4 Oct 2015
avatar mbabker mbabker - change - 4 Oct 2015
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 4 Oct 2015
Labels Added: ?
avatar zero-24 zero-24 - change - 5 Oct 2015
Category Libraries
avatar zero-24 zero-24 - change - 5 Oct 2015
Easy No Yes
avatar wilsonge
wilsonge - comment - 8 Oct 2015

Is this worth it when we are actually starting work on J4 and we would have to add code to work in conjunction with this when we add the 'real' DI?

avatar mbabker
mbabker - comment - 8 Oct 2015

Depends what the approach in J4 is gonna be. If we still end up with a
JFactory like object, this changes nothing. If we go 100% "real" DI, these
identified dependencies would shift to proper constructor arguments. Since
we don't have a factory for building these classes, this approach is the
best compromise we have now to try and do something.

On Thursday, October 8, 2015, George Wilson notifications@github.com
wrote:

Is this worth it when we are actually starting work on J4 and we would
have to add code to work in conjunction with this when we add the 'real' DI?


Reply to this email directly or view it on GitHub
#8012 (comment).

avatar rdeutz
rdeutz - comment - 8 Oct 2015

I am working on it following the way laravel is implementing it, so the application object extends the IoC. Atm I'm still fighting with the autoloading to make sure the old stuff is still working.

avatar wilsonge
wilsonge - comment - 10 Oct 2015

Personally I'd shove it in the constructor in J4 and not have to stress about keeping some vague sense of b/c with this then

avatar mbabker
mbabker - comment - 14 Oct 2015

We're already using this type of DI in JInput to optionally inject a JFilterInput object, so not unprecedented (and never refactored in the Framework). If I'm wasting my time just say so and I'll close it. Not like I don't have other PRs in the same boat.

avatar wilsonge
wilsonge - comment - 14 Oct 2015

OK I see we are using it to inject JFilterInput into JInput. Fix the conflicts, and I'll do my test.

avatar mbabker
mbabker - comment - 14 Oct 2015

Done

avatar KingLouis1 KingLouis1 - test_item - 24 Oct 2015 - Tested successfully
avatar KingLouis1 KingLouis1 - test_item - 24 Oct 2015 - Tested successfully
avatar KingLouis1
KingLouis1 - comment - 24 Oct 2015

I have tested this item :white_check_mark: successfully on 4dfefa8

No issues found


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8012.

avatar designbengel designbengel - test_item - 24 Oct 2015 - Tested successfully
avatar designbengel
designbengel - comment - 24 Oct 2015

I have tested this item :white_check_mark: successfully on 4dfefa8


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8012.

avatar matrikular matrikular - test_item - 24 Oct 2015 - Tested successfully
avatar matrikular
matrikular - comment - 24 Oct 2015

I have tested this item :white_check_mark: successfully on 4dfefa8

Looks good to me.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8012.

avatar zero-24 zero-24 - change - 24 Oct 2015
Milestone Added:
avatar zero-24 zero-24 - change - 24 Oct 2015
Milestone Added:
Status Pending Ready to Commit
avatar zero-24
zero-24 - comment - 24 Oct 2015

RTC. Thanks


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8012.

avatar joomla-cms-bot joomla-cms-bot - change - 24 Oct 2015
Labels Added: ?
avatar roland-d roland-d - change - 25 Oct 2015
Status Ready to Commit Closed
Closed_Date 0000-00-00 00:00:00 2015-10-25 17:02:47
Closed_By roland-d
avatar roland-d roland-d - close - 25 Oct 2015
avatar joomla-cms-bot joomla-cms-bot - close - 25 Oct 2015
avatar roland-d roland-d - reference | 4f865eb - 25 Oct 15
avatar roland-d roland-d - merge - 25 Oct 2015
avatar roland-d roland-d - close - 25 Oct 2015
avatar joomla-cms-bot joomla-cms-bot - change - 25 Oct 2015
Labels Removed: ?
avatar roland-d
roland-d - comment - 25 Oct 2015

Thanks everybody. This has now been merged into 3.5-dev.

avatar mbabker mbabker - head_ref_deleted - 25 Oct 2015
avatar wilsonge wilsonge - reference | 327edc1 - 31 Oct 15

Add a Comment

Login with GitHub to post a comment