Skip to content

Conversation

Upperfoot
Copy link
Contributor

Problem: Currently PHP 7.2 throws an error if count is used on a non-countable object.
Resolution: Check for variable Countableness.
Change: Check variable is an instanceof Countable.

Problem: Currently PHP 7.2 throws an error if count is used on a non-countable object.
Resolution: Check for variable Countableness.
Change: Check variable is an instanceof Countable.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.6%) to 94.026% when pulling 1fd9741 on Upperfoot:patch-1 into 9663472 on paquettg:master.

Copy link

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

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

Your changes change the semantics of the comparison, making the conditions always false. I suggested alternatives below.

{
// XPath index
if (count($rule['tag']) > 0 &&
if ($rule['tag'] instanceof Countable &&
Copy link

Choose a reason for hiding this comment

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

$rule['tag'] is compared with a string below, so this check is incorrect. I suggest checking against !empty instead of count.

if (count($rule['tag']) > 0 &&
if ($rule['tag'] instanceof Countable &&
count($rule['tag']) > 0 &&
$rule['key'] instanceof Countable &&
Copy link

Choose a reason for hiding this comment

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

$rule['key'] is compared with an int below, so this check is incorrect as well. Since it's supposed to be an index, I'd suggest to check $rule['key'] > 0

Copy link
Contributor Author

@Upperfoot Upperfoot left a comment

Choose a reason for hiding this comment

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

I don't understand why the original implementation would use count on strings or integers as opposed to arrays which it was designed for, is there a reason why?

@alcaeus
Copy link

alcaeus commented Nov 9, 2017

I don't understand why the original implementation would use count on strings or integers as opposed to arrays which it was designed for, is there a reason why?

Neither do I - especially since count($rule['key']) > 0 && is_numeric($rule['key']) makes no sense whatsoever.

Unfortunately, it looks like there hasn't been any activity here for quite some time.

@paquettg are you still maintaining this package? If not, would you consider somebody else to take over basic maintenance?

@DiederikvandenB
Copy link

I would love to see this issue resolved as it is breaking my applications.

@ssitdikov
Copy link

Is anybody will apply that PR?

@vikas5914
Copy link

Can anyone fork and apply the PR ?

@ThomasTr
Copy link

@paquettg could you please merge this PR as your bundle is actually not compatible with PHP 7.2

@alcaeus
Copy link

alcaeus commented Jan 10, 2018

I've tried reaching @paquettg a while ago, with no success. Thus, I've decided to create a fork and provide basic support for it: https://github.com/thesoftwarefanatics/php-html-parser.

Basic support means: we'll be fixing issues if they affect whatever we need of the library and we'll merge pull requests other contributors create. No active development is taking place - we're merely ensuring a dependency still works as it's supposed to.

Whether you're directly requiring this dependency or you're just getting it through another dependency, you can use this forked version by adding "thesoftwarefanatics/php-html-parser": "^1.8.0" to your composer.json - it will automatically replace the original library. Please note that we've dropped support for unsupported PHP versions (5.6 and 7.0) as well as HHVM. The new 1.8.0 release also supports PHP 7.2.

@crscheid
Copy link

Thanks @alcaeus. Lifesaver.

@coreation
Copy link

@alcaeus thanks a bunch!

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.