Cleanups, fixes and a bit of optimizations for site/components batch #3 by frankmayer · Pull Request #12292 · joomla/joomla-cms

frankmayer · 2016-10-03T15:54:41Z

com_content

Note: This is a single commit bundling all types of changes, since PR #12261 which had detailed commits, was rejected as a whole

- com_content Note: This is a single commit bundling all types of changes, since PR #12261 which had detailed commits, was rejected as a whole

andrepereiradasilva · 2016-10-03T19:22:54Z

components/com_content/helpers/query.php

-		$results = array ('select' => $select, 'join' => $join);
-
-		return $results;
+		return array ('select' => $select, 'join' => $join);


can we remove the space here?

andrepereiradasilva · 2016-10-03T19:27:03Z

components/com_content/models/category.php

+			if ($orderByPri === 'alpha' || $orderByPri === 'ralpha')
 			{
-				$this->_children = ArrayHelper::sortObjects($this->_children, 'title', ($params->get('orderby_pri') == 'alpha') ? 1 : (-1));
+				$this->_children = ArrayHelper::sortObjects($this->_children, 'title', ($params->get('orderby_pri') === 'alpha') ? 1 : (-1));


why are we getting orderby_pri param again here?

andrepereiradasilva · 2016-10-03T19:28:02Z

components/com_content/models/form.php

 					$associations[$tag] = (int) $associated->id;
 				}

 				$data['associations'] = $associations;


if you remove one if block you should remove one tab inside the if.

Noted as in other PR

andrepereiradasilva · 2016-10-03T19:32:00Z

components/com_content/views/category/tmpl/default_articles.php

 					<?php if (!empty($article->author) || !empty($article->created_by_alias)) : ?>
 						<?php $author = $article->author ?>
-						<?php $author = ($article->created_by_alias ? $article->created_by_alias : $author);?>
+						<?php $author = ($article->created_by_alias ?: $author);?>


why not remove the parenthisis here?
Also can you add an extra space before the php end tag?

andrepereiradasilva · 2016-10-03T19:32:52Z

components/com_content/views/category/view.html.php

 		// For blog layouts, preprocess the breakdown of leading, intro and linked articles.
 		// This makes it much easier for the designer to just interrogate the arrays.
-		if (($params->get('layout_type') == 'blog') || ($this->getLayout() == 'blog'))
+		if (($params->get('layout_type') === 'blog') || ($this->getLayout() === 'blog'))


why not remove the parenthisis here too?

@andrepereiradasilva

Made some changes as pointed out by @andrepereiradasilva

andrepereiradasilva · 2016-10-03T21:11:55Z

components/com_content/views/category/tmpl/default_articles.php

 					<?php if (!empty($article->author) || !empty($article->created_by_alias)) : ?>
 						<?php $author = $article->author ?>
-						<?php $author = ($article->created_by_alias ? $article->created_by_alias : $author);?>
+						<?php $author = $article->created_by_alias ?: $author; ?>


looking at this one again ...
why not , instead of too lines, use just:

<?php $author = $article->created_by_alias ?: $article->author; ?>

Found a bit more to refactor... coming up soon...

andrepereiradasilva · 2016-10-03T21:38:23Z

components/com_content/views/category/tmpl/default_children.php

+			$childNumItems = $child->getNumItems(true);
+			$childChildren = $child->getChildren();
+			$childChildrenCount = count($childChildren);
+			?>


the $childNumItems = $child->getNumItems(true); is only used if the language isRtland if $this->params->get('show_cat_num_articles', 1) so it should be inside that if.

the other two are only use if $this->maxLevel > 1 so should only be processed then.

so i don't agree with this change since will run this in all cases

as for the rest imo is fine

😄 yes, got confused by the template code... will revert...

andrepereiradasilva · 2016-10-03T22:08:30Z

I have tested this item ✅ successfully on c0b68fc

on code review

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12292.}

wilsonge · 2016-10-04T10:52:42Z

components/com_content/controllers/article.php

@@ -286,7 +286,6 @@ protected function getReturnPage()
 	 */
 	protected function postSaveHook(JModelLegacy $model, $validData = array())


Just kill the entire function. It will fall back to JControllerForm https://github.com/frankmayer/joomla-cms/blob/c0b68fcdf7735dbd4d86aaf24d87dfab9b53db8a/libraries/legacy/controller/form.php#L520

Yes, you are right. I didn't follow the trail, there. Thanks

@wilsonge

… to comment from @wilsonge

andrepereiradasilva · 2016-10-04T12:52:41Z

I have tested this item ✅ successfully on acd28f7

just a change at george request. still fine

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12292.}

# Conflicts: # components/com_content/helpers/association.php # components/com_content/helpers/route.php # components/com_content/router.php # components/com_content/views/article/tmpl/default.php # components/com_content/views/categories/tmpl/default_items.php # components/com_content/views/category/tmpl/blog_children.php # components/com_content/views/category/tmpl/default_articles.php # components/com_content/views/category/tmpl/default_children.php # components/com_content/views/featured/tmpl/default.php # components/com_content/views/featured/view.feed.php # components/com_content/views/form/tmpl/edit.php

andrepereiradasilva

some cs suggestions

andrepereiradasilva · 2016-12-13T18:15:13Z

components/com_content/models/category.php


-			if ($params->get('orderby_pri') == 'alpha' || $params->get('orderby_pri') == 'ralpha')
+			$orderByPri = $params->get('orderby_pri');
+			if ($orderByPri === 'alpha' || $orderByPri === 'ralpha')


cs: can we have an aempty line before the if statement?

of course :)

andrepereiradasilva · 2016-12-13T18:16:57Z

components/com_content/views/category/tmpl/blog_children.php

-				<?php endif; ?>
+				<?php if ($this->maxLevel > 1 && count($child->getChildren()) > 0) : ?>
+					<a href="#category-<?php echo $child->id;?>" data-toggle="collapse" data-toggle="button" class="btn btn-mini pull-right"><span class="icon-plus"></span></a>
+				<?php endif;?>


cs: please don't remove the empty space after the ; in php closing tags

Sorry, this happened while merging...

andrepereiradasilva · 2016-12-13T18:17:01Z

components/com_content/views/category/tmpl/blog_children.php

-				<?php endif; ?>
+				<?php if ($this->maxLevel > 1 && count($child->getChildren()) > 0) : ?>
+					<a href="#category-<?php echo $child->id;?>" data-toggle="collapse" data-toggle="button" class="btn btn-mini pull-right"><span class="icon-plus"></span></a>
+				<?php endif;?>


cs: please don't remove the empty space after the ; in php closing tags

Sorry, this happened while merging...

andrepereiradasilva · 2016-12-13T18:29:11Z

components/com_content/views/category/tmpl/blog_children.php

 				<?php echo $this->escape($child->title); ?></a>

-				<?php if (count($child->getChildren()) > 0 && $this->maxLevel > 1) : ?>
-					<a href="#category-<?php echo $child->id; ?>" data-toggle="collapse" data-toggle="button" class="btn btn-mini pull-right"><span class="icon-plus"></span></a>


cs: still one here

What is there?

you removed the space between ; and ?>

andrepereiradasilva · 2016-12-13T18:29:16Z

components/com_content/views/category/tmpl/blog_children.php

 				<?php endif; ?>

-				<?php if (count($child->getChildren()) > 0 && $this->maxLevel > 1) : ?>
-					<a href="#category-<?php echo $child->id; ?>" data-toggle="collapse" data-toggle="button" class="btn btn-mini pull-right"><span class="icon-plus"></span></a>


cs: still one here

What is there?

you removed the space between ; and ?>
(see lines below)

andrepereiradasilva · 2016-12-13T19:52:21Z

I have tested this item ✅ successfully on 0e94075

code review

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12292.}

# Conflicts: # components/com_content/helpers/query.php # components/com_content/helpers/route.php # components/com_content/models/articles.php # components/com_content/views/category/tmpl/default_articles.php # components/com_content/views/featured/view.feed.php

frankmayer · 2016-12-15T21:06:02Z

Fixed some conflicts that had occurred in the meantime.
Having merged the changes of above referenced PR's made this PR lighter and easier to test.
It is essentially ready to be merged, when the team finds the time and PR is OK.

andrepereiradasilva · 2016-12-15T21:08:50Z

components/com_content/controllers/article.php

-	 * @return  void
-	 *
-	 * @since   1.6
-	 */


Can we remove this?

Do you mean revert the removal?

forget it this is inhreited from https://github.com/joomla/joomla-cms/blob/staging/libraries/legacy/controller/form.php#L527

wojsmol · 2017-07-08T03:38:53Z

@frankmayer
Please look at #16757.

Cleanups, fixes and a bit of optimizations for site/components batch #3

fe3b1cb

- com_content Note: This is a single commit bundling all types of changes, since PR #12261 which had detailed commits, was rejected as a whole

joomla-cms-bot added the PR-staging label Oct 3, 2016

andrepereiradasilva reviewed Oct 3, 2016

View reviewed changes

Ch-Ch-Ch-Changes!

5c98de1

Made some changes as pointed out by @andrepereiradasilva

andrepereiradasilva reviewed Oct 3, 2016

View reviewed changes

A bit more...

13f5fee

andrepereiradasilva reviewed Oct 3, 2016

View reviewed changes

Revert

c0b68fc

wilsonge reviewed Oct 4, 2016

View reviewed changes

Removed empty function, as there is a fallback. Change made according…

acd28f7

… to comment from @wilsonge

andrepereiradasilva reviewed Dec 13, 2016

View reviewed changes

Included @andrepereiradasilva's suggestions

2d966e2

andrepereiradasilva reviewed Dec 13, 2016

View reviewed changes

frankmayer mentioned this pull request Dec 13, 2016

Unnecessary double quotes in site components #13183

Merged

Inserting whitespace before php closing tag

0e94075

This was referenced Dec 13, 2016

Simplify ternary with elvis in site component #13185

Merged

Merged unset() calls #13186

Merged

Removed one-time-use variables #13187

Merged

Use short syntax for applied operations #13188

Merged

Remove unnecessary parentheses in site components #13190

Merged

andrepereiradasilva reviewed Dec 15, 2016

View reviewed changes

andrepereiradasilva approved these changes Dec 15, 2016

View reviewed changes

wilsonge merged commit 22ea525 into joomla:staging Dec 18, 2016

wilsonge added this to the Joomla 3.7.0 milestone Dec 18, 2016

frankmayer deleted the site-com_content-com_contenthistory branch December 25, 2016 21:55

wojsmol mentioned this pull request Jul 8, 2017

Access ignored in List All Categories menu item #16757

Closed

		@@ -286,7 +286,6 @@ protected function getReturnPage()
		*/
		protected function postSaveHook(JModelLegacy $model, $validData = array())

Uh oh!

Conversation

frankmayer commented Oct 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrepereiradasilva Oct 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrepereiradasilva Oct 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrepereiradasilva commented Oct 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrepereiradasilva commented Oct 4, 2016

Uh oh!

andrepereiradasilva left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrepereiradasilva commented Dec 13, 2016

Uh oh!

frankmayer commented Dec 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wojsmol commented Jul 8, 2017

Uh oh!

Reviewers

Assignees

Labels

andrepereiradasilva Oct 3, 2016 •

edited

Loading

andrepereiradasilva Oct 3, 2016 •

edited

Loading