Skip to content
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

Fixes count() NULL bug for SequenceMatcher.php #1665

Closed
wants to merge 1 commit into from
Closed

Fixes count() NULL bug for SequenceMatcher.php #1665

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Oct 4, 2018

PHP 7.2 is stricter about invoking count() with parameters which are not countable:

https://secure.php.net/manual/en/migration72.incompatible.php

This case is triggered in SequenceMatcher, for example when reviewing
a initial history entry of a host template:

count(): Parameter must be an array or an object that implements Countable (SequenceMatcher.php:352)

#0 /usr/share/webapps/icingaweb2/modules/director/library/vendor/php-diff/lib/Diff/SequenceMatcher.php(352): Icinga\Application\ApplicationBootstrap->Icinga\Application\{closure}(2, 'count(): Parame...', '/usr/share/weba...', 352, Array)
#1 /usr/share/webapps/icingaweb2/modules/director/library/vendor/php-diff/lib/Diff/SequenceMatcher.php(469): Diff_SequenceMatcher->getMatchingBlocks()
#2 /usr/share/webapps/icingaweb2/modules/director/library/vendor/php-diff/lib/Diff/SequenceMatcher.php(525): Diff_SequenceMatcher->getOpCodes()
#3 /usr/share/webapps/icingaweb2/modules/director/library/vendor/php-diff/lib/Diff.php(176): Diff_SequenceMatcher->getGroupedOpcodes(5)
#4 /usr/share/webapps/icingaweb2/modules/director/library/vendor/php-diff/lib/Diff/Renderer/Html/Array.php(70): Diff->getGroupedOpcodes()
#5 /usr/share/webapps/icingaweb2/modules/director/library/vendor/php-diff/lib/Diff/Renderer/Html/SideBySide.php(55): Diff_Renderer_Html_Array->render()
#6 /usr/share/webapps/icingaweb2/modules/director/library/vendor/php-diff/lib/Diff.php(104): Diff_Renderer_Html_SideBySide->render()
#7 /usr/share/webapps/icingaweb2/modules/director/library/Director/ConfigDiff.php(62): Diff->render(Object(Diff_Renderer_Html_SideBySide))
#8 /usr/share/webapps/icingaweb2/modules/director/library/Director/ConfigDiff.php(55): Icinga\Module\Director\ConfigDiff->renderHtmlSideBySide()
#9 /usr/share/webapps/icingaweb2/modules/director/library/Director/ConfigDiff.php(47): Icinga\Module\Director\ConfigDiff->renderHtml()
#10 /usr/share/webapps/icingaweb2/modules/director/library/vendor/ipl/Html/Html.php(171): Icinga\Module\Director\ConfigDiff->render()
#11 /usr/share/webapps/icingaweb2/modules/director/library/vendor/ipl/Html/Html.php(171): dipl\Html\Html->render()
#12 /usr/share/webapps/icingaweb2/modules/director/library/vendor/ipl/Html/BaseElement.php(105): dipl\Html\Html->render()
#13 /usr/share/webapps/icingaweb2/modules/director/library/vendor/ipl/Html/BaseElement.php(133): dipl\Html\BaseElement->renderContent()
#14 /usr/share/webapps/icingaweb2/modules/director/library/vendor/ipl/Html/Html.php(259): dipl\Html\BaseElement->render()
#15 /usr/share/webapps/icingaweb2/modules/director/library/vendor/ipl/Zf1/SimpleViewRenderer.php(47): dipl\Html\Html->__toString()
#16 /usr/share/webapps/icingaweb2/modules/director/library/vendor/ipl/Zf1/SimpleViewRenderer.php(66): dipl\Zf1\SimpleViewRenderer->render()
#17 /usr/share/webapps/icingaweb2/library/vendor/Zend/Controller/Action/HelperBroker.php(272): dipl\Zf1\SimpleViewRenderer->postDispatch()
#18 /usr/share/webapps/icingaweb2/library/vendor/Zend/Controller/Action.php(518): Zend_Controller_Action_HelperBroker->notifyPostDispatch()
#19 /usr/share/webapps/icingaweb2/library/Icinga/Web/Controller/Dispatcher.php(76): Zend_Controller_Action->dispatch('activityAction')
#20 /usr/share/webapps/icingaweb2/library/vendor/Zend/Controller/Front.php(937): Icinga\Web\Controller\Dispatcher->dispatch(Object(Icinga\Web\Request), Object(Icinga\Web\Response))
#21 /usr/share/webapps/icingaweb2/library/Icinga/Application/Web.php(300): Zend_Controller_Front->dispatch(Object(Icinga\Web\Request), Object(Icinga\Web\Response))
#22 /usr/share/webapps/icingaweb2/library/Icinga/Application/webrouter.php(104): Icinga\Application\Web->dispatch()
#23 /usr/share/webapps/icingaweb2/public/index.php(4): require_once('/usr/share/weba...')
#24 {main}

I fixed this like in #1458.

Signed-off-by: phreeek [email protected]

PHP 7.2 is stricter about invoking count() with parameters which are not countable:

 https://secure.php.net/manual/en/migration72.incompatible.php

This case is triggered in SequenceMatcher, for example when reviewing
a initial history entry of a host template:

count(): Parameter must be an array or an object that implements Countable (SequenceMatcher.php:352)

I fixed this like in #1458.

Signed-off-by: phreeek <[email protected]>
@Thomas-Gelf
Copy link
Contributor

Thanks, @phreeek! This should definitively be fixed. It's vendored code, so before touching it I'd love to get a better understanding of what's going wrong. To me the main error here is that something changes these properties to a non-array, and I haven't been able to see where this should take place. Did you dig deeper into that class? Does "reviewing the initial history" mean it should happen all the times I click on the History Tab of a newly created Host Template?

@Thomas-Gelf
Copy link
Contributor

ping

@Al2Klimov
Copy link
Member

Fortunately there's already a PR addressing this: chrisboulton/php-diff#50

Unfortunately the lib seems not to be maintained: chrisboulton/php-diff#53

Maybe we should consider switching the lib?

@Thomas-Gelf
Copy link
Contributor

This has been fixed with 525b316 for v1.5.0. I guess @phreeek (user no longer exists) was testing with 1.4 and I wasn't able to understand why this patch should be required on the master branch at that time. Got no feedback, the PR you linked has been applied more than two years ago, we can close this.

@JBlond
Copy link

JBlond commented Oct 30, 2021

If you are still using this. I maintain the successor repo https://github.com/JBlond/php-diff

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants