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

PHP 7.2 Compat - Fix count() uses #18438

Merged
merged 1 commit into from Nov 30, 2017

Conversation

Projects
None yet
9 participants
@mbabker
Member

mbabker commented Oct 30, 2017

Summary of Changes

PHP 7.2 disallows the use of the count() function on non countable items (arrays or objects implementing the Countable interface). We have places in our API where null values were being returned then pushed into a count() call, this PR addresses three uses found on a cursory click through a CMS install using PHP 7.2.0RC5.

Testing Instructions

To verify:

  • Install Joomla without sample data then click to the login page. You get a countable warning coming from the Updater.php file by way of the plugin which checks for updates and sends an email notification triggering an update check. This is caused by properties in the CollectionAdapter class not being initialized as empty arrays and therefore when no updates are found the properties are still set as null versus being array.
  • After logging in, the two articles modules will give a countable warning coming from the com_content articles model. This is caused by a state value not setting an appropriate default if the state value is null.
  • Install Joomla with the testing sample data and navigate to the frontend "Single Contact" page. You get a countable warning coming from the similar tags module. This is caused by the module's helper method returning null in some conditions instead of simply returning an empty array.

Apply the patch and repeat these steps.

(If you do not reinstall before item 1, you will need to clear the update check timestamps for the plg_system_updatenotification plugin params and the Joomla! Core update site).

Expected result

Data is displayed without error.

Actual result

Errors as a result of trying to count a non-countable item.

@@ -267,7 +267,7 @@ protected function getListQuery()
}
// Filter by categories and by level
$categoryId = $this->getState('filter.category_id');
$categoryId = $this->getState('filter.category_id', array());
$level = $this->getState('filter.level');

This comment has been minimized.

@ggppdk

ggppdk Oct 30, 2017

Contributor

hhmm my PR has added this PHP 7.2 issue

Just with the above default value 'array(),' if someone uses
index.php?option=com_content&view=articles&filter[category_id]=0
then we will get same warning,
so maybe better to replace the following code with

$categoryId = !is_array($categoryId)
	? ($categoryId ? array($categoryId) : array())
	: $categoryId;
@ggppdk

ggppdk Oct 30, 2017

Contributor

hhmm my PR has added this PHP 7.2 issue

Just with the above default value 'array(),' if someone uses
index.php?option=com_content&view=articles&filter[category_id]=0
then we will get same warning,
so maybe better to replace the following code with

$categoryId = !is_array($categoryId)
	? ($categoryId ? array($categoryId) : array())
	: $categoryId;

This comment has been minimized.

@zero-24

zero-24 Nov 4, 2017

Contributor

@mbabker can you implement that? ;)

@zero-24

zero-24 Nov 4, 2017

Contributor

@mbabker can you implement that? ;)

@alikon

This comment has been minimized.

Show comment
Hide comment
@alikon

alikon Nov 2, 2017

Contributor

I have tested this item successfully on 26ee013


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

Contributor

alikon commented Nov 2, 2017

I have tested this item successfully on 26ee013


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

@roland-d

This comment has been minimized.

Show comment
Hide comment
@roland-d

roland-d Nov 26, 2017

Contributor

I have tested this item successfully on ca74bf9

All three cases were showing the warning running PHP 7.2RC6. After applying the patch the warnings were gone.


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

Contributor

roland-d commented Nov 26, 2017

I have tested this item successfully on ca74bf9

All three cases were showing the warning running PHP 7.2RC6. After applying the patch the warnings were gone.


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

@Quy Quy referenced this pull request Nov 29, 2017

Closed

php 7.2 support count() #18925

@810

This comment has been minimized.

Show comment
Hide comment
@810

810 Nov 30, 2017

Contributor

I have tested this item successfully on ca74bf9


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

Contributor

810 commented Nov 30, 2017

I have tested this item successfully on ca74bf9


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

@franz-wohlkoenig

This comment has been minimized.

Show comment
Hide comment
@franz-wohlkoenig

franz-wohlkoenig Nov 30, 2017

Ready to Commit after two successful tests.

franz-wohlkoenig commented Nov 30, 2017

Ready to Commit after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC label Nov 30, 2017

@rdeutz rdeutz merged commit 78938a8 into joomla:staging Nov 30, 2017

5 checks passed

JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
hound No violations found. Woof!

@joomla-cms-bot joomla-cms-bot removed the RTC label Nov 30, 2017

@rdeutz rdeutz added this to the Joomla 3.8.3 milestone Nov 30, 2017

@mbabker mbabker deleted the mbabker:php72-countable branch Dec 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment