-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Simplify ternary with elvis in site component #13185
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
Simplify ternary with elvis in site component #13185
Conversation
|
I have tested this item ✅ successfully on 414f378 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13185. |
|
I have tested this item ✅ successfully on 414f378 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13185. |
|
RTC This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13185. |
…nent # Conflicts: # components/com_config/model/cms.php # components/com_content/views/archive/tmpl/default_items.php # components/com_content/views/article/tmpl/default_links.php
|
Conflicts resolved. Could you pls review and merge if OK? |
| } | ||
|
|
||
| $method = $input->get('method') ? $input->get('method') : 'get'; | ||
| $method = $input->get('method') ?: 'get'; |
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 wanted to point out that there is no need for a ternary / elvis operator here at all. The method should be retrieved / set via the second parameter (default) that can be passed to the get method of JInput.
get(string $name, mixed $default = null, string $filter = 'cmd') : mixed
I would do a PR for that, if there are no objections.
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.
👍
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.
@matrikular Of course 😄
Summary of Changes
This PR is part of a set to try to separate some of the changes done in some of my previous batch PR's for site/components, which are still on hold (#12290, #12292, #12293, #12294).
Testing Instructions
None, should not change behavior
Documentation Changes Required
None.