Work on com_finder (both admin and site) #12254

frankmayer · 2016-10-02T14:42:15Z

Summary of Changes

Best practices, some optimization and cleaning up.

Testing Instructions

This should need only code review

Also Todo'd two things for someone to check

…and it might cause trouble...

andrepereiradasilva · 2016-10-03T21:59:17Z

administrator/components/com_finder/views/filters/tmpl/default.php

 				$canCreate  = $user->authorise('core.create',     'com_finder');
 				$canEdit    = $user->authorise('core.edit',       'com_finder');
+
+				foreach ($this->items as $i => $item) :
 				$canCheckin = $user->authorise('core.manage',     'com_checkin') || $item->checked_out == $user->get('id') || $item->checked_out == 0;
 				$canChange  = $user->authorise('core.edit.state', 'com_finder') && $canCheckin;
 				?>


you could also move $user->authorise('core.manage', 'com_checkin') and $user->authorise('core.edit.state', 'com_finder') to outside the for since the $user does not change inside the for.

Actually I can't, because the second depends on the first, which may have different results because of the $item of the loop. Or am I getting this wrong?

But I could move the user->get outside the loop.

i don't mean all of it i mean just the

$user->authorise('core.manage', 'com_checkin')

And the

$user->authorise('core.edit.state', 'com_finder')

can be outside the cycle

Sorry, missed that. Yes, that is a nice idea. Let's try that.

andrepereiradasilva · 2016-10-03T21:59:33Z

administrator/components/com_finder/views/filters/view.html.php

 	 */
 	protected $state;

 	/**
 	 * The total number of items
 	 *
-	 * @var  object
+	 * @var  int


andrepereiradasilva · 2016-10-03T21:59:41Z

administrator/components/com_finder/views/filter/view.html.php

 	 */
 	protected $state;

 	/**
+	 * The total indexed items
+	 *
+	 * @var  int


andrepereiradasilva · 2016-10-03T22:00:26Z

administrator/components/com_finder/views/index/view.html.php

 	 */
 	protected $state;

 	/**
 	 * The total number of items
 	 *
-	 * @var  object
+	 * @var  int


…function calls out of the loop or reduce calls inside the loop.

andrepereiradasilva · 2016-10-06T11:23:57Z

@frankmayer when you change tabs identation is more hard for testers to make a code review.

I normally append ?w=1 (ignore whitespaces changes) to the "Files changed" tab URL (Ex: https://github.com/joomla/joomla-cms/pull/12254/files?w=1) in those cases to be more easy to spot the differences, but i think not all people know this "trick".

So my sugestion is don't chnange code identation in this PRs

frankmayer · 2016-12-13T13:58:34Z

@andrepereiradasilva As far as I can tell changed indentation on purpose was in one file only.
The other changes of indentation was the removal of nested ifs, resulting in the code of that group being moved to the left.
btw. nice parameter hint

# Conflicts: # components/com_finder/models/search.php

andrepereiradasilva · 2016-12-13T18:33:53Z

administrator/components/com_finder/views/filter/view.html.php

@@ -20,31 +20,48 @@ class FinderViewFilter extends JViewLegacy
 	 * The filter object
 	 *
 	 * @var  FinderTableFilter
+	 *
+	 * @since  2.5


this (and below) were actually added in 3.6.2 see https://github.com/frankmayer/joomla-cms/commit/20c18465afd26df6d7f2c7b7f46124104edfc70f#diff-9d1414e13ac35d173505ccd27d5fc1f5R20

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

administrator/components/com_finder/views/filter/view.html.php

+	 *
+	 * @since  2.5
+	 */
+	protected $total;


__DEPLOY_VERSION__

andrepereiradasilva · 2016-12-13T18:37:10Z

administrator/components/com_finder/views/filters/tmpl/default.php

-				</tr>
+					$canCheckIn = $userAuthoriseCoreManage || $item->checked_out == $userId || $item->checked_out == 0;
+					$canChange = $userAuthoriseCoreEditState && $canCheckIn;
+					$escapedTitle = $this->escape($item->title);


cs: please ident the equal signs

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

administrator/components/com_finder/views/filters/view.html.php

@@ -20,34 +20,44 @@ class FinderViewFilters extends JViewLegacy
 	 * An array of items
 	 *
 	 * @var  array
+	 *
+	 * @since  2.5


this (and below) was actually added in 3.6.1 see 20c1846

andrepereiradasilva · 2016-12-13T18:42:34Z

administrator/components/com_finder/views/index/view.html.php

@@ -22,41 +22,53 @@ class FinderViewIndex extends JViewLegacy
 	 * An array of items
 	 *
 	 * @var  array
+	 *
+	 * @since  2.5


this (and below) was actually added in 3.6.1 see 20c1846

andrepereiradasilva · 2016-12-13T18:42:56Z

administrator/components/com_finder/views/maps/view.html.php

@@ -22,34 +22,44 @@ class FinderViewMaps extends JViewLegacy
 	 * An array of items
 	 *
 	 * @var  array
+	 *
+	 * @since  2.5


this (and below) was actually added in 3.6.1 see 20c1846

andrepereiradasilva · 2016-12-13T18:43:19Z

administrator/components/com_finder/views/statistics/view.html.php

@@ -20,6 +20,8 @@ class FinderViewStatistics extends JViewLegacy
 	 * The index statistics
 	 *
 	 * @var  JObject
+	 *
+	 * @since  2.5


this was actually added in 3.6.1 see 20c1846

andrepereiradasilva · 2016-12-13T18:45:41Z

components/com_finder/views/search/view.html.php

+
+	protected $total;
+
+	protected $pagination;


please add doc blocks

I removed one protected property, as it seemed not be used.

andrepereiradasilva · 2016-12-13T22:44:03Z

administrator/components/com_finder/views/filter/view.html.php

 	 */
 	protected $filter;

 	/**
 	 * The JForm object
 	 *
 	 * @var  JForm
+	 *
+	 * @since  2.5


andrepereiradasilva · 2016-12-13T22:44:07Z

administrator/components/com_finder/views/filter/view.html.php

-	 * @var  object
+	 * @var  JObject|boolean
+	 *
+	 * @since  2.5


andrepereiradasilva · 2016-12-13T22:44:10Z

administrator/components/com_finder/views/filter/view.html.php

-	 * @var  object
+	 * @var  mixed
+	 *
+	 * @since  2.5


andrepereiradasilva · 2016-12-13T22:44:43Z

administrator/components/com_finder/views/index/view.html.php

 	 */
 	protected $items;

 	/**
 	 * The pagination object
 	 *
 	 * @var  JPagination
+	 *
+	 * @since  2.5


andrepereiradasilva · 2016-12-13T22:44:47Z

administrator/components/com_finder/views/index/view.html.php

 	 */
 	protected $pagination;

 	/**
 	 * The state of core Smart Search plugins
 	 *
 	 * @var  array
+	 *
+	 * @since  2.5


andrepereiradasilva · 2016-12-13T22:45:37Z

administrator/components/com_finder/views/maps/view.html.php

 	 */
 	protected $sidebar;

 	/**
 	 * The model state
 	 *
 	 * @var  object
+	 *
+	 * @since  2.5


andrepereiradasilva · 2016-12-13T22:45:41Z

administrator/components/com_finder/views/maps/view.html.php

 	 */
 	protected $state;

 	/**
 	 * The total number of items
 	 *
 	 * @var  object
+	 *
+	 * @since  2.5


andrepereiradasilva · 2016-12-13T22:46:22Z

components/com_finder/views/search/view.html.php

+     *
+     * @var    integer
+     *
+     * @since  3.6.1


__DEPLOY_VERSION__

andrepereiradasilva · 2016-12-13T22:46:41Z

components/com_finder/views/search/view.html.php

+     *
+     * @var    array
+     *
+     * @since  3.6.1


__DEPLOY_VERSION__

andrepereiradasilva · 2016-12-13T22:47:04Z

components/com_finder/views/search/view.html.php

 	protected $state;

-	protected $user;


why is this removed? is B/C to remove a protected property?

Yes, it could be. I was overeagerly cleaning up and noticed, it wasn't used in the context of this file or related tmpl. Will revert.

Quy · 2017-06-10T00:14:42Z

administrator/components/com_finder/models/maps.php

@@ -345,16 +345,13 @@ public function publish(&$pks, $value = 1)
 		{
 			$table->reset();

-			if ($table->load($pk))
+			if ($table->load($pk) && !$this->canEditState($table))


Dedent block below.

Quy · 2017-06-10T00:16:13Z

administrator/components/com_finder/tables/filter.php

@@ -168,12 +168,12 @@ public function publish($pks = null, $state = 1, $userId = 0)
 		}

 		// If checkin is supported and all rows were adjusted, check them in.
-		if ($checkin && (count($pks) == $this->_db->getAffectedRows()))
+		if ($checkin && (count($pks) === $this->_db->getAffectedRows()))


Remove outside parentheses (count($pks) === $this->_db->getAffectedRows())

Quy · 2017-06-10T00:17:58Z

administrator/components/com_finder/views/index/tmpl/default.php

@@ -118,7 +118,7 @@
 						<?php echo JHtml::_('date', $item->indexdate, JText::_('DATE_FORMAT_LC4')); ?>
 					</td>
 					<td class="center hidden-phone hidden-tablet">
-						<?php if (intval($item->publish_start_date) or intval($item->publish_end_date) or intval($item->start_date) or intval($item->end_date)) : ?>
+						<?php if ((int) $item->publish_start_date or (int) $item->publish_end_date or (int) $item->start_date or (int) $item->end_date) : ?>


Change or to ||

Quy · 2017-06-10T00:18:36Z

administrator/components/com_finder/views/statistics/tmpl/default.php

@@ -34,7 +34,7 @@
 						<?php
 						$lang_key    = 'PLG_FINDER_STATISTICS_' . str_replace(' ', '_', $type->type_title);
 						$lang_string = JText::_($lang_key);
-						echo ($lang_string == $lang_key) ? $type->type_title : $lang_string;
+						echo ($lang_string === $lang_key) ? $type->type_title : $lang_string;


Remove parentheses.

frankmayer · 2017-06-10T00:30:58Z

Thanks for reviewing @Quy

ghost · 2017-06-10T04:18:45Z

@frankmayer is this PR to be tested by Code review?

Quy · 2017-06-10T13:03:23Z

components/com_finder/models/search.php

@@ -425,7 +425,7 @@ protected function getResultsTotal()
 					$temp = $this->_db->loadObjectList();

 					// Set the more flag to true if any of the sets equal the limit.
-					$more = (count($temp) === $limit) ? true : false;
+					$more = (count($temp) === $limit);


Sorry found more. Remove parentheses. Do the same 3 more times below.

Quy · 2017-06-10T13:04:01Z

@franz-wohlkoenig yes code review.

ghost · 2017-06-10T13:24:46Z

@Quy thanks for Info. @frankmayer can you please wrote Test Instructions "Code review" in first Comment for willing Tester?

frankmayer · 2017-06-10T15:39:03Z

@Quy, @franz-wohlkoenig : Done and Done. Thanks

# Conflicts: # administrator/components/com_finder/views/filters/tmpl/default.php

frankmayer · 2017-06-13T15:44:41Z

Conflicts resolved. Please test
Code review only should do.

andrepereiradasilva

just some minor comments
rest seems fine on code review

andrepereiradasilva · 2017-06-23T23:29:10Z

administrator/components/com_finder/models/filter.php


-		return $total;
+		return $db->loadResult();


small improv: you could use return $db->setQuery($query)->loadResult(); and remove the above line

Totally missed your comments here. Commit coming up.

andrepereiradasilva · 2017-06-23T23:36:53Z

components/com_finder/views/search/view.html.php

@@ -9,22 +9,65 @@

 defined('_JEXEC') or die;

+


please remove this empty line

andrepereiradasilva · 2017-06-28T22:19:58Z

I have tested this item ✅ successfully on 222ab4c

code review

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

Quy · 2017-06-29T02:29:53Z

I have tested this item ✅ successfully on 222ab4c

Code review

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

ghost · 2017-06-29T04:32:11Z

RTC after two successful tests.

frankmayer added 16 commits October 2, 2016 05:13

Case mismatch in function call

80f6fdb

Elvis

97a4c6d

Fix nested positive ifs

e563437

Fix static method invocation via ->

8cee0d2

" to '

f7ed1bd

Remove unnecessary parentheses

20cec49

Merged isset()

a24c4f8

Inline redundant value

e2c3744

Move disconnected code outside the foreach loop.

e0aeaa4

Fixed Docblocks

9cfbd61

Replaced preg_replace() with str_replace()

6bf0c52

Replaced PHP4 type-casting

25d572e

Type-safe string comparisons

642d58f

More type-safe comparisons

ceb6a45

Removed unused call and variable.

2897582

Also Todo'd two things for someone to check

Reverse preg_replace() replacement because there was a limit in preg …

9cc93ed

…and it might cause trouble...

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

Codesniffer

2c72520

andrepereiradasilva reviewed Oct 3, 2016

View reviewed changes

frankmayer added 3 commits October 4, 2016 01:14

Fix integer

b8d3840

Changes proposed by @andrepereiradasilva and two more by me, to get …

5e814cb

…function calls out of the loop or reduce calls inside the loop.

Shouldn't this be written this way?

b29d7f9

Merge branch 'staging' into com_finder

4ea6fef

# Conflicts: # components/com_finder/models/search.php

andrepereiradasilva reviewed Dec 13, 2016

View reviewed changes

Some extra work according to @andrepereiradasilva's notes

bc7d4dc

andrepereiradasilva reviewed Dec 13, 2016

View reviewed changes

frankmayer added 2 commits December 14, 2016 01:05

Some more changes from @andrepereiradasilva's comments

f2da7fa

Some more changes from @andrepereiradasilva's comments

723ed89

Merge branch 'staging' into com_finder

8e7d1ad

Quy reviewed Jun 10, 2017

View reviewed changes

Additional changes according to reviewer's comments

9bf494b

Quy reviewed Jun 10, 2017

View reviewed changes

Additional changes according to reviewer's comments

54e9bad

Merge branch 'staging' into com_finder

5b2fd9d

# Conflicts: # administrator/components/com_finder/views/filters/tmpl/default.php

andrepereiradasilva reviewed Jun 23, 2017

View reviewed changes

frankmayer added 3 commits June 29, 2017 00:00

Merge branch 'staging' into com_finder

01b79f4

Changes, according to @andrepereiradasilva's suggestions

6685117

Forgot to commit this one

557ad78

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

rdeutz added this to the Joomla 3.8.0 milestone Jul 6, 2017

mbabker changed the base branch from staging to 3.8-dev July 19, 2017 12:25

mbabker merged commit ac7f0d5 into joomla:3.8-dev Jul 19, 2017

joomla-cms-bot added PR-3.8-dev and removed RTC This Pull Request is Ready To Commit labels Jul 19, 2017

mbabker removed the PR-staging label Jul 19, 2017

frankmayer deleted the com_finder branch August 23, 2017 19:07

Work on com_finder (both admin and site) #12254

Work on com_finder (both admin and site) #12254

Conversation

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

Summary of Changes

Testing Instructions

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrepereiradasilva 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrepereiradasilva commented Oct 6, 2016

frankmayer commented Dec 13, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrepereiradasilva Dec 13, 2016 • 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

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

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

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 Jun 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankmayer commented Jun 10, 2017

ghost commented Jun 10, 2017

Choose a reason for hiding this comment

Quy commented Jun 10, 2017

ghost commented Jun 10, 2017

frankmayer commented Jun 10, 2017

frankmayer commented Jun 13, 2017

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

Choose a reason for hiding this comment

andrepereiradasilva commented Jun 28, 2017

Quy commented Jun 29, 2017

ghost commented Jun 29, 2017

frankmayer commented Oct 2, 2016 •

edited by joomla-cms-bot

andrepereiradasilva Oct 3, 2016 •

edited

andrepereiradasilva Dec 13, 2016 •

edited

Quy Jun 10, 2017 •

edited