Skip to content

PHP 7.2 Compat - Fix count() uses#18438

Merged
rdeutz merged 1 commit intojoomla:stagingfrom
mbabker:php72-countable
Nov 30, 2017
Merged

PHP 7.2 Compat - Fix count() uses#18438
rdeutz merged 1 commit intojoomla:stagingfrom
mbabker:php72-countable

Conversation

@mbabker
Copy link
Copy Markdown
Contributor

@mbabker 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.

$categoryId = $this->getState('filter.category_id');
$categoryId = $this->getState('filter.category_id', array());
$level = $this->getState('filter.level');

Copy link
Copy Markdown
Contributor

@ggppdk ggppdk Oct 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Copy Markdown
Contributor

@zero-24 zero-24 Nov 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mbabker can you implement that? ;)

@alikon
Copy link
Copy Markdown
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
Copy link
Copy Markdown
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.

@Quy Quy mentioned this pull request Nov 29, 2017
@810
Copy link
Copy Markdown
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.

@ghost
Copy link
Copy Markdown

ghost commented Nov 30, 2017

Ready to Commit after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Nov 30, 2017
@rdeutz rdeutz merged commit 78938a8 into joomla:staging Nov 30, 2017
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Nov 30, 2017
@rdeutz rdeutz added this to the Joomla 3.8.3 milestone Nov 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants