? ? Success

User tests: Successful: Unsuccessful:

avatar photodude
photodude
24 Mar 2016

Pull Request for Issue "JApplicationWebClient was not extend from, nor aliased to, Framework WebClient: thus duplicate code"

Summary of Changes

  • Remove duplicate code by aliasing JApplicationWebClient to the Framework
  • Removes unit test for JApplicationWebClientTest.php, With the removal of JApplicationWebClient in favor of the version found in the framework, this unit test is no longer needed since the framework is already unit tested.

Testing Instructions

  1. Add patch to 3.5.0 or staging https://docs.joomla.org/Testing_Joomla!_patches
  2. Navigate to the backend and frontend with one or more browsers/devices and be sure nothing is broken (web apps only)
  3. Report your result back here with the J!Tracker Application
avatar photodude photodude - open - 24 Mar 2016
avatar photodude photodude - change - 24 Mar 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 24 Mar 2016
Labels Added: ?
avatar photodude photodude - change - 24 Mar 2016
The description was changed
avatar brianteeman brianteeman - change - 24 Mar 2016
Category Libraries
avatar wilsonge
wilsonge - comment - 24 Mar 2016

I'd take it one step further - #7554 (that conflicts now so I'll close it in favour of this but....)

avatar photodude
photodude - comment - 24 Mar 2016

@wilsonge In completely removing the JApplicationWebClient file and proxying to the framework class, can we mark JApplicationWebClient as a deprecated method?

avatar wilsonge
wilsonge - comment - 24 Mar 2016

It won't show as a class anymore in IDE's

avatar photodude
photodude - comment - 24 Mar 2016

But there wouldn't be any "notice" that JApplicationWebClient is a deprecated method and could be removed as an alias in the future, and to use Joomla\Application\Web\WebClient directly.

avatar wilsonge
wilsonge - comment - 24 Mar 2016

No - but that's no different to JRegistry...

avatar photodude
photodude - comment - 24 Mar 2016

But was that the right choice to alias JRegistry to the framework? IMHO it binds our hands in keeping an ever growing alias list (since we can't mark an alias as deprecated). Whereas with just extending we can mark the empty class deprecated to be dropped in a future version. Which would give 3rd party developers a heads up and give us the flexibility to not maintain an alias list with no path for removing unnecessary ancient aliases.

I'm not sure if marking the class as deprecated is an important stance. On the other hand, I do think it's very important to drop duplicated code. So whatever direction is more likely to get consideration and move towards getting rid of the duplicated code is truly good with me.

avatar mbabker
mbabker - comment - 24 Mar 2016

Aliases can be deprecated. There just is no way to make it raise a log message or have it spit out on api.joomla.org without putting a "real" class back. And honestly in the case of JRegistry that can't be done without B/C breaks (you either had to alias the whole thing or you couldn't namespace it because of the class inheritance on the format objects, making the non-namespaced JRegistryFormatJson extend its namespaced counterpart would have broken $object instanceof JRegistryFormat checks, with it all aliased that didn't happen).

avatar photodude
photodude - comment - 24 Mar 2016

So in this case, should we extend with the option to "raise a log message" and have api.joomla.org spit out a notice; or should we Alias?

My opinions are listed above... whatever is more likely to get accepted so we can get rid of the duplicated code.

avatar mbabker
mbabker - comment - 24 Mar 2016

I'd say alias given the precedent already set with the other classes, but that's just me. If you can completely alias it without breaking things (I'm talking actual code use, not someone's ability to get references in the IDE), go with aliasing. If you can't do it without breaking something, leave a "dead class" in place.

For reference, I looked into this once. You can't reliably log anything to the deprecated category before the debug plugin is instantiated because that's when the logger for that is set up. The aliases are loaded into the autoloader long before that. Anything that happens between when the index.php files are triggered to this line in JApplicationCms can't be logged in a conventional manner. As for alias versus "dead class", each has pros and cons (the main things are IDE detection and the ability to document it with parsers like phpDocumentor).

avatar joomla-cms-bot joomla-cms-bot - change - 24 Mar 2016
Labels Added: ?
avatar photodude photodude - change - 24 Mar 2016
Title
Remove duplicate code and extend JApplicationWebClient from Framework
Remove duplicate code by aliasing JApplicationWebClient to Framework
avatar photodude
photodude - comment - 24 Mar 2016

"completely alias it without breaking things" I guess wins out...

Should be good to go for testing now that it's aliased to the framework.

avatar photodude
photodude - comment - 19 Apr 2016

@rdeutz I believe this one should also be considered for 3.5.2

avatar rdeutz
rdeutz - comment - 19 Apr 2016

@photodude because you asked I looked into it :-) I think this should go into 3.6.x because it adds a new function and so it is a API change (or better extension). I can't see side effect but who knows. Anyway we need tests for this change.

avatar rdeutz rdeutz - change - 19 Apr 2016
Milestone Added:
avatar mbabker mbabker - change - 8 May 2016
Status Pending Ready to Commit
Labels
avatar mbabker
mbabker - comment - 8 May 2016

This is fine to merge, RTC


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

avatar joomla-cms-bot joomla-cms-bot - change - 8 May 2016
Labels Added: ?
avatar wilsonge wilsonge - change - 8 May 2016
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2016-05-08 21:38:58
Closed_By wilsonge
avatar wilsonge
wilsonge - comment - 8 May 2016

Merged :) thanks!

avatar joomla-cms-bot joomla-cms-bot - change - 8 May 2016
Labels Removed: ?

Add a Comment

Login with GitHub to post a comment