[#36512] - [4] Application Constants
- Closed
- 6 Mar 2022
- Medium
- Build: 4.0-dev
- # 36512
- Diff
- PhilETaylor:clientconstants
User tests: Successful: Unsuccessful:
Summary of Changes
In modern PHP the correct way to test if something is something else is with instanceof E.g
if (Factory::getApplication() instanceof SiteApplication){
// it is!
}For legacy reasons Joomla CMS has 4 applications (and I know of at least 2 third party developers with their own "Application")
0 = Site
1 = Administrator
2 = Installation
3 = Api
4 = Console/CLI
Throughout Joomla there are many calls to check if the currently running app is one of these with methods like $app->isClient('site') where a method is passed a string...
This string is actually a hard coded string. prone to typos. and is actually, really, a constant, that doesn't change. Its a defining constant.
The modern PHP way of passing around strings as constants is to actually make them class constants that can be accessed with \Joomla\CMS\Application\SiteApplication::CLIENT for example.
This improves static analysis of code, this removes the chance of a typo (or using 'admin' instead of 'administrator'), This allows IDE auto completion and is... just the right thing to be doing.
This PR introduces Class Constants for all applications and implements them wherever they used to be strings in method calls.
/**
* @const string The client name
*
* @since __DEPLOY_VERSION__
*/
public const CLIENT = 'api';
/**
* @const int The client id
*
* @since __DEPLOY_VERSION__
*/
public const CLIENT_ID = 3;This change is 100% backward compatible as we are just replacing a string with a constant (of a string). Extensions and 3PD code that uses the old strings is still going to work - as long as Joomla doesn't decide to rename the client names (and I doubt they ever would!)
CLIENT_ID has been added as an integer constant for use in the same manner
I see this as a stepping stop to modernising the code base to eventually, Joomla 5 .. Joomla 6.... remove the client names and use instanceof throughout - as that is modern PHP best practice. But I consider that a step to far and is a b/c change for sure. It might never get that far, but this PR is a step in the right direction.
Testing Instructions
Code review
Use Joomla - see what breaks.
Actual result BEFORE applying this Pull Request
Everything works, but I have to remember how to spell "administrator" correctly.
Static analysis of code doesn't work well.
Legacy non-modern handling of parameters which are actually constants, as strings.
Expected result AFTER applying this Pull Request
A tiny step along the path to being a more modern code base. About 0.00000001% better.
IDE Auto completion
Static Anaylsis works better.
Documentation Changes Required
Educate everyone to use the new class constants instead of hard coding the client names.
Instead of
if ($app->isClient('site'))
{
Session::checkToken('get') or die(Text::_('JINVALID_TOKEN'));
}use the class constants
if ($app->isClient(SiteApplication::CLIENT))
{
Session::checkToken('get') or die(Text::_('JINVALID_TOKEN'));
}| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Administration com_associations com_categories com_config com_contact com_content com_fields com_finder com_joomlaupdate com_languages com_login com_menus |
| Labels |
Added:
?
|
||
Does this PR make joomla more secure, faster or easier to work with?
Does this PR make joomla more secure, faster or easier to work with?
For a developer - YES!
Ignoring the developer experience has been the death of Joomla. Its not all about pushing pixels and admin templates.
It have a sens, but make thing more complex for Joe Average than it already is.
I would not do it, for now.
It have a sens
That is why we are replacing hard coded strings with constants everywhere in Joomla 4. See the Providers for evidence of that.
but make thing more complex for Joe Average than it already is.
Rubbish. If "Joe Average" non-joomla-code developer wishes to still use 'site' and 'administrator' as hard coded strings, THIS PR DOESNT STOP THEM DOING THAT. This PR is backward compatible, and contains ZERO NEW DEPRECATIONS
Its about LEADING BY EXAMPLE, Joomla Core Code should be modernised. Then "Joe Average" developer will copy and paste and follow suit - eventually - as that's always been the case.
Joomla Core Contributors can be encouraged by people like you, at the PR stage, to commit with modern standards - such as using constants instead of hand crafted strings.
Heck, if I were a moderator I would not even allow any calls to a method when a call to instanceof is more performant and modern PHP (and fully compatible with the PHP restrictions of Joomla!) (And please dont come back with the 10 year old StackOverflow messages that say instanceof is slow... instanceofis a language construct and therefore faster than a method call, that has to then look up a class property and do a comparison)
Joe Average Developers and Joomla Core Developers GAIN IDE Completion, making it more simple, less prone to errors, Static Analysis and a whole host of other benefits such as being able to see where the constants are used throughout the code with a single click.
I stopped short of deprecating anything - so if Joe Average wants to code like its 1999 he can do - but this raises the standard for Joomla Core ONLY. It is hopped then that Joe Average will then follow suit over the next decade.
This type of change is not without precedent in Joomla.
We no longer pass around class names as hard coded strings but we use the \Joomla\f\q\c\n::class syntax instead. Look at the Providers for an example of that.
And an example (currently about to be replaced with the class constant in my PR) where we do it wrong still:
Now is the time for Joomla code to grow up, instead of carrying around old ways of doing it. PHP 8+ is a grown up PHP - its time Joomla Core Code was held to a higher standard. We can't just keep adding a J Prefix to everything and carrying a legacy two-tier, standard of coding for the next decade.
Just as an example, If we went full hog and went to using instanceof instead of $app->isClient() we would be 22-58% more performant... (take the actual numbers with a pinch of salt, fact is in every benchmark test using instanceof language construct is faster than $app->isClient()
But remember - this is commentary - this PR DOESNT ENFORCE ANYTHING NEW.... (that comes later. IF you ever decide to remove isClient().. )
And for completeness sake - this PR doesnt slow down code, in fact comparing you will always see that comparing the class constant the way this PR provides, is actually quicker
Obviously small numbers - but equally not a single run was slower
Also this PR was not done with performance in mind - it is done with DX (Developer Experience) in mind and Modernising codebase.
okay okay, I see :)
will see what other will say
As we load the applications through a container, we can even think about to introduce interfaces for every app, so others can implement a site application without the requirement to extend it.
Closing as too modern and professional a way to code for the hobby Joomla project... Joomla will forever pass around random strings instead of constants apparently...
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2022-03-06 21:03:12 |
| Closed_By | ⇒ | PhilETaylor | |
| Labels |
Added:
Conflicting Files
?
Removed: ? |
||
Thanks for the down thumb @brianteeman - and to think, my next PR was to add back your request for isSite/isAdmin!
This PR is about modernizing the code - we cannot live in the past for another decade. As a non PHP developer, I do not expect you to understand why this PR is actually correct, and passing around hand coded strings is wrong in 2022. Check out any other professional PHP project and you will see.