[#8971] - Introduce new method isClient($identifier) in JApplicationCms class
- Fixed in Code Base
- 1 Oct 2016
- Medium
- Build: 3.7.x
- # 8971
- Diff
- izharaazmi:app-client
- Success continuous-integration/travis-ci/pr The Travis CI build passed Details
User tests: Successful: Unsuccessful:
Introduction
This PR introduces a new method JApplicationCms::isClient($identifier) as a substitute for isSite and isAdmin methods. Hence, remove usage of descendant class reference in the JApplication (legacy) and JApplicationCms for better OO structure and support more extensibility.
Since the current implementation essentially assumes its two subclasses JApplicationSite and JApplicationAdministrator which should be avoided as much possible.
This PR also enables more subclasses derived from JApplicationCms to make use of this method. Such as in the Installation app of the Joomla installer package, we currently cannot perform a test something like $app->isInstallation();.
But this PR allows us to do the test like $app->isClient('installation');
Feature Testing
Since this PR does not change any existing usage of existing methods the cms should not behave any different.
Run the following code in frontend, backend and installation applications respectively and match the output.
// Get instance of current application.
$app = JFactory::getApplication();
// Should give bool(true) in frontend, bool(false) otherwise.
var_dump($app->isClient('site'));
// var_dump($app->isClient(0));
// Should give bool(true) in backend, bool(false) otherwise.
var_dump($app->isClient('administrator'));
// var_dump($app->isClient(1));
// Should give bool(true) in Joomla installer, bool(false) otherwise.
var_dump($app->isClient('installation'));
// var_dump($app->isClient(2));EDIT: Removed support for client id (integer) with 84163c5.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Labels |
Added:
?
|
||
| Labels |
Added:
?
|
||
I'd drop the integer and leave the name. There are very few places that actually refer to the client IDs throughout the code base; the name is more favored.
Exactly, I wonder why do we even support IDs, why not just deprecate without a substitute!
And, the PR... your suggestion sounds good to me. Should I make this changes or wait for more inputs from other fellow members?
| Labels | |||
| Category | ⇒ | Libraries |
Can we make this a part of 3.6 release? Somebody available to test this please?
| Category | Libraries | ⇒ | Libraries Unit Tests |
| Labels | |||
I have tested this item
I have tested this issue @icampus PBF with the proposed change and it works as its supposed to. I followed the suggested steps and IsClient() returned the correct value in each case.
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8971.
| Category | Libraries Unit Tests | ⇒ | Feature Request Libraries Unit Tests |
| Labels |
Added:
?
|
||
| Labels | |||
I have tested this item
I have followed the suggested steps and outputs are correct for each case.
@icampus PBF
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8971.
| Status | Pending | ⇒ | Ready to Commit |
| Labels | |||
| Labels |
Added:
?
|
||
@izharaazmi could you have a look at the conflicts and resolve them, thanks!
| Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2016-10-01 12:03:21 |
| Closed_By | ⇒ | rdeutz |
zero-24
- | Labels |
Removed:
?
|
||
1) I'd leave legacy JApplication alone, it's going away and already out of use in the CMS.
2) Personally, I would not accept mixed argument types on the new method. I'd either use the client ID or the client name, the latter would be more easily recognized. Methods that accept mixed parameter types (unless it's something acting as a data store like JObject or the Registry classes) looks like a code smell, you're essentially building two methods into one.