Skip to content
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

Clean up ContentViewCategory #19237

Closed
wants to merge 3 commits into from
Closed

Clean up ContentViewCategory #19237

wants to merge 3 commits into from

Conversation

joomdonation
Copy link
Contributor

Pull Request for Issue # .

Summary of Changes

This PR makes some clean up to ContentViewCategory classes:

  1. Use namespace classes instead of it's alias

  2. Move $dispatcher = JEventDispatcher::getInstance(); out of the loop, so we get a bit performance improvement

  3. Remove code to set view layout as it is set on parent class code already https://github.com/joomla/joomla-cms/blob/staging/libraries/src/MVC/View/CategoryView.php#L212-L223

  4. Remove some un-used variables (like $id, $pathway), reuse-existing variable (no need for $menu = $menu->getActive() when we have $active variable keep active menu item before)

Testing Instructions

Code review should be enough. For human testing, please create a menu item to display articles from a category (Category Blog layout menu option for example), check and make sure articles still being displayed like before

@@ -79,7 +81,8 @@ public function display($tpl = null)
$numLinks = $params->def('num_links', 4);
$this->vote = JPluginHelper::isEnabled('content', 'vote');

JPluginHelper::importPlugin('content');
PluginHelper::importPlugin('content');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 82 has a prefix J. This one does not. Should they be the same and with or without J?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without check line 13

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without J as @dgt41 pointed out. I updated the code. If #19234 and #19239 are accepted, I will still need to make another PR to complete clean up code in this class.

@Quy
Copy link
Contributor

Quy commented Jan 8, 2018

I have tested this item ✅ successfully on 02f1b74


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19237.

@joomla-cms-bot joomla-cms-bot added the Language Change This is for Translators label Jan 15, 2018
@joomdonation
Copy link
Contributor Author

joomdonation commented Jan 15, 2018

Ouch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language Change This is for Translators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants