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

frankmayer · 2016-10-03T15:55:34Z

com_mailto
com_newsfeeds
com_search

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

- com_mailto - com_newsfeeds - com_search 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:14:08Z

components/com_mailto/controller.php

@@ -135,7 +135,6 @@ public function send()

 		// Build the message to send
 		$msg  = JText::_('COM_MAILTO_EMAIL_MSG');
-		$link = $link;


nice one! 😉

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

components/com_newsfeeds/views/newsfeed/view.html.php

@@ -139,7 +139,7 @@ public function display($tpl = null)
 		// Check the access to the newsfeed
 		$levels = $user->getAuthorisedViewLevels();

-		if (!in_array($item->access, $levels) or ((in_array($item->access, $levels) and (!in_array($item->category_access, $levels)))))
+		if (!in_array($item->access, $levels) or in_array($item->access, $levels) and (!in_array($item->category_access, $levels)))


why not replace the and and or here?

Slipped through...

also maybe you need to add one parenthisis here. not sure tough

Yes, precedence maybe.. I mostly picked low hanging fruit.. That's possibly why this one "slipped through" 😄

andrepereiradasilva · 2016-10-03T21:15:21Z

I have tested this item ✅ successfully on c34e7bf

on code review

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

# Conflicts: # components/com_newsfeeds/helpers/route.php # components/com_newsfeeds/router.php # components/com_newsfeeds/views/categories/tmpl/default_items.php # components/com_newsfeeds/views/category/tmpl/default_items.php # components/com_newsfeeds/views/newsfeed/tmpl/default.php

andrepereiradasilva · 2016-12-13T18:24:51Z

I have tested this item ✅ successfully on 2b4d825

code review

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

# Conflicts: # components/com_newsfeeds/helpers/route.php # components/com_newsfeeds/views/newsfeed/tmpl/default.php # components/com_newsfeeds/views/newsfeed/view.html.php

frankmayer · 2016-12-15T21:18:41Z

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.

Quy · 2017-05-28T19:05:35Z

components/com_newsfeeds/controller.php

@@ -36,7 +36,7 @@ public function display($cachable = false, $urlparams = false)

 		$user = JFactory::getUser();

-		if ($user->get('id') || ($this->input->getMethod() == 'POST' && $vName == 'category' ))
+		if ($user->get('id') || ($this->input->getMethod() === 'POST' && $vName === 'category' ))


Remove extra space 'category' )).

Quy · 2017-05-28T19:08:25Z

components/com_newsfeeds/models/newsfeed.php

@@ -127,7 +127,7 @@ public function &getItem($pk = null)

 				// Check for published state if filter set.

-				if (((is_numeric($published)) || (is_numeric($archived))) && (($data->published != $published) && ($data->published != $archived)))
+				if ((is_numeric($published) || is_numeric($archived)) && (($data->published != $published) && ($data->published != $archived)))


Change (($data->published != $published) && ($data->published != $archived)) to $data->published != $published && $data->published != $archived.

Quy · 2017-05-28T19:11:06Z

components/com_newsfeeds/router.php

@@ -58,20 +58,18 @@ public function build(&$query)
 		}

 		// Are we dealing with a newsfeed that is attached to a menu item?
-		if (isset($query['view']) && ($mView == $query['view']) and (isset($query['id'])) and ($mId == (int) $query['id']))
+		if (isset($query['view'], $query['id']) && ($mView == $query['view']) && ($mId == (int) $query['id']))


Change && ($mView == $query['view']) && ($mId == (int) $query['id']) to && $mView == $query['view'] && $mId == (int) $query['id'].

Quy · 2017-05-28T19:17:55Z

components/com_newsfeeds/views/category/tmpl/default_items.php

@@ -20,9 +20,9 @@
 <?php else : ?>

 <form action="<?php echo htmlspecialchars(JUri::getInstance()->toString(), ENT_COMPAT, 'UTF-8'); ?>" method="post" name="adminForm" id="adminForm">
-	<?php if ((($this->params->get('filter_field') != 'hide') || ($this->params->get('filter_field') != '0')) || $this->params->get('show_pagination_limit')) :?>
+	<?php if ((($this->params->get('filter_field') !== 'hide') || ($this->params->get('filter_field') != '0')) || $this->params->get('show_pagination_limit')) :?>


Remove parentheses.

Quy · 2017-05-28T19:18:01Z

components/com_newsfeeds/views/category/tmpl/default_items.php

 	<fieldset class="filters btn-toolbar">
-		<?php if (($this->params->get('filter_field') != 'hide') || ($this->params->get('filter_field') != '0')) :?>
+		<?php if (($this->params->get('filter_field') !== 'hide') || ($this->params->get('filter_field') != '0')) :?>


Remove parentheses

Quy · 2017-05-28T19:20:01Z

components/com_newsfeeds/views/newsfeed/tmpl/default.php


-		if ($lang->isRtl() && $myrtl == 0)
+	$isRtl= $lang->isRtl();


# Conflicts: # components/com_newsfeeds/views/category/tmpl/default_items.php # components/com_search/controller.php # components/com_search/views/search/view.html.php

- Some more changes in updated stuff

Quy · 2017-05-28T22:42:05Z

I have tested this item ✅ successfully on 6cc4c65
Code review

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

andrepereiradasilva

one question with strict compare a integer type with mixed(?) type. please check
the rest seems fine

andrepereiradasilva · 2017-05-31T17:37:10Z

components/com_newsfeeds/router.php

 					{
 						return $child->id;
 					}
 				}
 				else
 				{
-					if ($child->id == (int) $segment)
+					if ($child->id === (int) $segment)


are you sure the categories child id is a integer?
see https://github.com/joomla/joomla-cms/blob/staging/libraries/legacy/categories/categories.php#L722 for instance

Not sure. Will check. According to the property's docblock it is an integer, but I am not sure if we can trust that. See: https://github.com/joomla/joomla-cms/blob/staging/libraries/legacy/categories/categories.php#L395

andrepereiradasilva · 2017-06-01T09:46:48Z

I have tested this item ✅ successfully on 5b88330

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

frankmayer · 2017-06-02T14:00:06Z

@franz-wohlkoenig RTC please , thank you

ghost · 2017-06-02T14:03:20Z

RTC after two successful tests.

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

25ca8c7

- com_mailto - com_newsfeeds - com_search 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

Missed to convert and and or

c34e7bf

frankmayer added 2 commits December 13, 2016 17:34

Some more fixes after conflict resolution

2b4d825

frankmayer closed this Dec 13, 2016

frankmayer reopened this Dec 13, 2016

Merge branch 'staging' into site-com_mailto-com_newsfeeds-com_search

8864cff

# Conflicts: # components/com_newsfeeds/helpers/route.php # components/com_newsfeeds/views/newsfeed/tmpl/default.php # components/com_newsfeeds/views/newsfeed/view.html.php

andrepereiradasilva approved these changes Dec 15, 2016

View reviewed changes

Quy reviewed May 28, 2017

View reviewed changes

frankmayer added 2 commits May 29, 2017 00:22

Merge branch 'staging' into site-com_mailto-com_newsfeeds-com_search

59ff2e6

# Conflicts: # components/com_newsfeeds/views/category/tmpl/default_items.php # components/com_search/controller.php # components/com_search/views/search/view.html.php

- Changed some things according to reviewer's suggestions

6cc4c65

- Some more changes in updated stuff

andrepereiradasilva reviewed May 31, 2017

View reviewed changes

frankmayer added 2 commits June 1, 2017 02:08

Merge branch 'staging' into site-com_mailto-com_newsfeeds-com_search

2748148

Reversed a change according to reviewer's correct comment.

5b88330

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jun 2, 2017

rdeutz added this to the Joomla 3.7.3 milestone Jun 8, 2017

rdeutz merged commit 778153e into joomla:staging Jun 8, 2017

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jun 8, 2017

frankmayer deleted the site-com_mailto-com_newsfeeds-com_search branch June 8, 2017 10:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

frankmayer commented Oct 3, 2016 •

edited by joomla-cms-bot

andrepereiradasilva Oct 3, 2016

andrepereiradasilva Oct 3, 2016

frankmayer Oct 3, 2016 •

edited

andrepereiradasilva Oct 3, 2016

frankmayer Oct 3, 2016

andrepereiradasilva commented Oct 3, 2016

andrepereiradasilva commented Dec 13, 2016

frankmayer commented Dec 15, 2016

Quy May 28, 2017

Quy May 28, 2017 •

edited

Quy May 28, 2017 •

edited

Quy May 28, 2017

Quy May 28, 2017

Quy May 28, 2017

Quy commented May 28, 2017 •

edited

andrepereiradasilva left a comment

andrepereiradasilva May 31, 2017

frankmayer May 31, 2017

andrepereiradasilva commented Jun 1, 2017

frankmayer commented Jun 2, 2017

ghost commented Jun 2, 2017

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

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

Conversation

frankmayer commented Oct 3, 2016 • edited by joomla-cms-bot

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankmayer Oct 3, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrepereiradasilva commented Oct 3, 2016

andrepereiradasilva commented Dec 13, 2016

frankmayer commented Dec 15, 2016

Choose a reason for hiding this comment

Quy May 28, 2017 • edited

Choose a reason for hiding this comment

Quy May 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Quy commented May 28, 2017 • edited

andrepereiradasilva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrepereiradasilva commented Jun 1, 2017

frankmayer commented Jun 2, 2017

ghost commented Jun 2, 2017

frankmayer commented Oct 3, 2016 •

edited by joomla-cms-bot

frankmayer Oct 3, 2016 •

edited

Quy May 28, 2017 •

edited

Quy May 28, 2017 •

edited

Quy commented May 28, 2017 •

edited