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

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


 		// 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

 		$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

 		$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

 				// 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


 		// 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


 <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

+	<?php if ((($this->params->get('filter_field') !== 'hide') || ($this->params->get('filter_field') != '0')) || $this->params->get('show_pagination_limit')) :?>
 	<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

+	$direction = ' ';

-		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

 				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

Uh oh!

Conversation

frankmayer commented Oct 3, 2016 • edited by joomla-cms-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frankmayer 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!

andrepereiradasilva commented Dec 13, 2016

Uh oh!

frankmayer commented Dec 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Quy May 28, 2017 • 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!

Quy May 28, 2017 • 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!

Quy commented May 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

andrepereiradasilva commented Jun 1, 2017

Uh oh!

frankmayer commented Jun 2, 2017

Uh oh!

ghost commented Jun 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

frankmayer commented Oct 3, 2016 •

edited by joomla-cms-bot

Loading

frankmayer Oct 3, 2016 •

edited

Loading

Quy May 28, 2017 •

edited

Loading

Quy May 28, 2017 •

edited

Loading

Quy commented May 28, 2017 •

edited

Loading