Problem/Motivation

This happens, if the user submits the compare form without selecting any revisions.

TypeError: count(): Argument #1 ($value) must be of type Countable|array, null given in count() (line 143 of /var/www/html/p/.finpedia-grp/web/modules/contrib/diff/src/Plugin/views/field/DiffPluginBase.php)
#0 /var/www/html/p/.finpedia-grp/web/modules/contrib/diff/src/Plugin/views/field/DiffPluginBase.php(143): count(NULL)
#1 /var/www/html/p/.finpedia-grp/web/modules/contrib/diff/src/Plugin/views/field/DiffFrom.php(66): Drupal\diff\Plugin\views\field\DiffPluginBase->loadEntityFromDiffFormKey('')
#2 /var/www/html/p/.finpedia-grp/web/core/modules/views/src/Form/ViewsFormMainForm.php(183): Drupal\diff\Plugin\views\field\DiffFrom->viewsFormSubmit(Array, Object(Drupal\Core\Form\FormState))
#3 /var/www/html/p/.finpedia-grp/web/core/modules/views/src/Form/ViewsForm.php(195): Drupal\views\Form\ViewsFormMainForm->submitForm(Array, Object(Drupal\Core\Form\FormState))
#4 [internal function]: Drupal\views\Form\ViewsForm->submitForm(Array, Object(Drupal\Core\Form\FormState))
#5 /var/www/html/p/.finpedia-grp/web/core/lib/Drupal/Core/Form/FormSubmitter.php(114): call_user_func_array(Array, Array)
#6 /var/www/html/p/.finpedia-grp/web/core/lib/Drupal/Core/Form/FormSubmitter.php(52): Drupal\Core\Form\FormSubmitter->executeSubmitHandlers(Array, Object(Drupal\Core\Form\FormState))
#7 /var/www/html/p/.finpedia-grp/web/core/lib/Drupal/Core/Form/FormBuilder.php(597): Drupal\Core\Form\FormSubmitter->doSubmitForm(Array, Object(Drupal\Core\Form\FormState))
#8 /var/www/html/p/.finpedia-grp/web/core/lib/Drupal/Core/Form/FormBuilder.php(325): Drupal\Core\Form\FormBuilder->processForm('views_form_revi...', Array, Object(Drupal\Core\Form\FormState))
#9 /var/www/html/p/.finpedia-grp/web/core/lib/Drupal/Core/Form/FormBuilder.php(224): Drupal\Core\Form\FormBuilder->buildForm(Object(Drupal\views\Form\ViewsForm), Object(Drupal\Core\Form\FormState))
#10 /var/www/html/p/.finpedia-grp/web/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php(2257): Drupal\Core\Form\FormBuilder->getForm(Object(Drupal\views\Form\ViewsForm), Object(Drupal\views\ViewExecutable), Array)
#11 [internal function]: Drupal\views\Plugin\views\display\DisplayPluginBase->elementPreRender(Array)
#12 /var/www/html/p/.finpedia-grp/web/core/lib/Drupal/Core/Security/DoTrustedCallbackTrait.php(101): call_user_func_array(Array, Array)
#13 /var/www/html/p/.finpedia-grp/web/core/lib/Drupal/Core/Render/Renderer.php(788): Drupal\Core\Render\Renderer->doTrustedCallback(Array, Array, 'Render #pre_ren...', 'exception', 'Drupal\\Core\\Ren...')
#14 /var/www/html/p/.finpedia-grp/web/core/lib/Drupal/Core/Render/Renderer.php(374): Drupal\Core\Render\Renderer->doCallback('#pre_render', Array, Array)
#15 /var/www/html/p/.finpedia-grp/web/core/lib/Drupal/Core/Render/Renderer.php(446): Drupal\Core\Render\Renderer->doRender(Array)
#16 /var/www/html/p/.finpedia-grp/web/core/lib/Drupal/Core/Render/Renderer.php(204): Drupal\Core\Render\Renderer->doRender(Array, false)
#17 /var/www/html/p/.finpedia-grp/web/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php(242): Drupal\Core\Render\Renderer->render(Array, false)
#18 /var/www/html/p/.finpedia-grp/web/core/lib/Drupal/Core/Render/Renderer.php(580): Drupal\Core\Render\MainContent\HtmlRenderer->Drupal\Core\Render\MainContent\{closure}()
#19 /var/www/html/p/.finpedia-grp/web/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php(243): Drupal\Core\Render\Renderer->executeInRenderContext(Object(Drupal\Core\Render\RenderContext), Object(Closure))
#20 /var/www/html/p/.finpedia-grp/web/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php(132): Drupal\Core\Render\MainContent\HtmlRenderer->prepare(Array, Object(Symfony\Component\HttpFoundation\Request), Object(Drupal\Core\Routing\CurrentRouteMatch))
#21 /var/www/html/p/.finpedia-grp/web/core/lib/Drupal/Core/EventSubscriber/MainContentViewSubscriber.php(90): Drupal\Core\Render\MainContent\HtmlRenderer->renderResponse(Array, Object(Symfony\Component\HttpFoundation\Request), Object(Drupal\Core\Routing\CurrentRouteMatch))
#22 [internal function]: Drupal\Core\EventSubscriber\MainContentViewSubscriber->onViewRenderArray(Object(Symfony\Component\HttpKernel\Event\ViewEvent), 'kernel.view', Object(Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher))
#23 /var/www/html/p/.finpedia-grp/web/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php(142): call_user_func(Array, Object(Symfony\Component\HttpKernel\Event\ViewEvent), 'kernel.view', Object(Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher))
#24 /var/www/html/p/.finpedia-grp/vendor/symfony/http-kernel/HttpKernel.php(174): Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch(Object(Symfony\Component\HttpKernel\Event\ViewEvent), 'kernel.view')
#25 /var/www/html/p/.finpedia-grp/vendor/symfony/http-kernel/HttpKernel.php(81): Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object(Symfony\Component\HttpFoundation\Request), 1)
#26 /var/www/html/p/.finpedia-grp/web/core/lib/Drupal/Core/StackMiddleware/Session.php(58): Symfony\Component\HttpKernel\HttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#27 /var/www/html/p/.finpedia-grp/web/core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php(48): Drupal\Core\StackMiddleware\Session->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#28 /var/www/html/p/.finpedia-grp/web/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php(48): Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#29 /var/www/html/p/.finpedia-grp/web/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php(51): Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#30 /var/www/html/p/.finpedia-grp/vendor/stack/builder/src/Stack/StackedHttpKernel.php(23): Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#31 /var/www/html/p/.finpedia-grp/web/core/lib/Drupal/Core/DrupalKernel.php(718): Stack\StackedHttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#32 /var/www/html/p/.finpedia-grp/web/index.php(19): Drupal\Core\DrupalKernel->handle(Object(Symfony\Component\HttpFoundation\Request))
#33 {main}

Proposed resolution

Adding a validate callback for the form to make sure, that revisions are selected.

Issue fork diff-3360589

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

jurgenhaas created an issue. See original summary.

jurgenhaas opened merge request !31

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented
Status: Active » Needs review
jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented

Another related issue is this warning:

Warning: Undefined array key 591307 in Drupal\diff\Controller\PluginRevisionController->buildRevisionsNavigation() (line 253 of /var/www/html/web/modules/contrib/diff/src/Controller/PluginRevisionController.php)

which happens, if left and right revision are either identical or if the left revision is newer than the right revision. And that produces a watchdog record for EVERY revision ID, i.e. in our case hundreds of thousands. ;-)

I've updated the MR to also validate this case and produce an applicable error message.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Uhm, for globally "every revision id", not even limited to the entity id?
Or you have hundreds of thousands revision ids on this single entity?

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented

Well, I wasn't able to verify that in all detail. What happened is in \Drupal\diff\Controller\PluginRevisionController::buildRevisionsNavigation this code block:

      $i = 0;
      // Find the previous revision.
      while ($left_revision_id > $revision_ids[$i]) {
        $i += 1;
      }

From $revision_ids[$i] we got 756 watchdog entries in sequential order for the counter, while the node only has 2 revisions. TBH, I didn't debug how that came about, I only prevented that from happening again with the validation of the form in the MR.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Interesting, i think this part should be improved with an array sort / search, without loop.

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented

Just checked the case which happened with a URL like /node/23017/revisions/view/166027/166014/visual_inline, so the diff between the rev IDs is 166027 - 166014 = 13, so not related to the 756 watchdog entries.

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented

i think this part should be improved with an array sort / search, without loop.

Yeah, I think that makes sense. Still, the form validation should still be present to let the user know when they selected something meaningless.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Sure this should be fixed.

But from the way i used it, i thought from / to direction don't matter. As long as they are different.
Are you sure about bigger / smaller check is needed?

Maybe we can just swap the revisions when the sequence is wrong?
Making it just work is better than telling the user to choose something different...

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented

Are you sure about bigger / smaller check is needed?

At the moment, yes, the order matters. I'm able to reproduce that. However, swapping them should resolve that automatically, I just wasn't sure if there were multiple places where this needed to be checked. However, the validation that they are not equal is still required, as we can't auto-correct that input error.

heddn made their first commit to this issue’s fork.

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Version: 8.x-1.x-dev » 2.x-dev
Status: Needs review » Needs work
Issue tags: +Needs tests

For this to be reviewed and accepted, please review https://git.drupalcode.org/project/diff#contribution-guidelines. Specifically we'll want to see some tests.