-
-
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
[5.3] Refactoring views to directly call models (com_guidedtours to com_newsfeeds) #44164
base: 5.3-dev
Are you sure you want to change the base?
Conversation
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.
Only the state looks strange the rest seems ok.
/** @var InstallerModel $model */ | ||
$model = $this->getModel(); | ||
|
||
// Get data from the model. | ||
$state = $this->get('State'); | ||
$this->state = $state = $model->getState(); | ||
|
||
// Are there messages to display? | ||
$showMessage = false; | ||
$this->showMessage = false; | ||
|
||
if (\is_object($state)) { | ||
$message1 = $state->get('message'); | ||
$message2 = $state->get('extension_message'); | ||
$showMessage = ($message1 || $message2); | ||
$message1 = $state->get('message'); | ||
$message2 = $state->get('extension_message'); | ||
$this->showMessage = ($message1 || $message2); | ||
} | ||
|
||
$this->showMessage = $showMessage; | ||
$this->state = &$state; | ||
|
||
$this->addToolbar(); |
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.
That change looks a bit strange, can we always expect that the state is a Registry or Object?
If not the reference in line 87 may have a special purpose?
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.
Yes, it always is a Registry object, however you are right, the $state in line 79 is wrong. Fixing it now.
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.
Done
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.
See line 84.
Summary of Changes
This PR refactors the views to get the model object and then directly call the methods of the model instead of going through the
View::get()
indirection. This refactors the views of the backend components from com_guidedtours to com_newsfeeds.Testing Instructions
Everything should work the same.
Link to documentations
Please select:
Documentation link for docs.joomla.org:
No documentation changes for docs.joomla.org needed
Pull Request link for manual.joomla.org:
No documentation changes for manual.joomla.org needed