User tests: Successful: Unsuccessful:
Pull Request for Issue "JApplicationWebClient was not extend from, nor aliased to, Framework WebClient: thus duplicate code"
Status | New | ⇒ | Pending |
Labels |
Added:
?
|
Category | ⇒ | Libraries |
It won't show as a class anymore in IDE's
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.
No - but that's no different to JRegistry...
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.
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).
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.
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).
Labels |
Added:
?
|
Title |
|
"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.
@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.
Milestone |
Added: |
Status | Pending | ⇒ | Ready to Commit |
Labels |
This is fine to merge, RTC
Labels |
Added:
?
|
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 |
Merged :) thanks!
Labels |
Removed:
?
|
I'd take it one step further - #7554 (that conflicts now so I'll close it in favour of this but....)