-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactored Factory::getUser() to ::getApplication()->getIdentity() #37321
Conversation
* @param array $messages The contents of the messages to be logged | ||
* @param string $messageLanguageKey The language key of the message | ||
* @param string $context The context of the content passed to the plugin | ||
* @param int $userId ID of user perform the action, usually ID of current logged in user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert.
Thanks for your pr. Honestly I would like to see a change to introduce a user service which can be injected into the view, to get a user from rather exchanging a static function call with another. |
A small addition:
|
Point of interest: |
Generally speaking this is OK. Worth noting that behaviours aren't exactly identical as noted in #36448 so you can't do a full auto-replace in an IDE. @laoneo HTMLView's at least will also be hard coupled to the application for the active template (https://github.com/joomla/joomla-cms/blob/4.1-dev/libraries/src/MVC/View/HtmlView.php#L377). So unless we plan to start injecting that whilst obviously the statics aren't ideal - it might be that we inject the application into the view in the constructor going forwards anyhow. Even if we inject the user because there's going to be authentication checks throughout the view probably makes more sense in the constructor than as a service there anyhow. |
What about the UserFactory? |
@pe7er You might want to close this PR because we now use a different approach:
|
@@ -44,7 +44,7 @@ class ActionlogModel extends BaseDatabaseModel | |||
*/ | |||
public function addLog($messages, $messageLanguageKey, $context, $userId = null) | |||
{ | |||
$user = Factory::getUser($userId); | |||
$user = Factory::getApplication()->getIdentity($userId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for information the change here is not right. The getIdentity method does not accept any parameter like you passed here. If we want to replace it, we will need to use UserFactory, something like:
$user = Factory::getContainer()->get(UserFactoryInterface::class)->loadUserById($userId);
Agree, this one should be closed as users will be injected into views and models. |
This pull request has automatically rebased to 4.2-dev. |
@joomdonation thanks for your explanation and for your PR to solve this in a better way closing this one... |
Joomla 4.1 uses the deprecated
Factory::getUser()
. I've changed it in a couple of files toFactory::getApplication()->getIdentity()
.Summary of Changes
This PR changes
Factory::getUser
in 5 files toFactory::getApplication()->getIdentity
.In some cases I added Exception to the code block and as name space.
$user = Factory::getUser($userId);
to
$user = Factory::getApplication()->getIdentity($userId);
$user = Factory::getUser();
to
$user = Factory::getApplication()->getIdentity();
if (!Factory::getUser()->authorise('core.admin'))
to
if (!Factory::getApplication()->getIdentity()->authorise('core.admin'))
Testing Instructions
Please do a code inspection.
It should not change any functionality and Joomla should continue work correctly.