Preview of content - Notice: Undefined offset: 0 in _quickedit_entity_is_latest_revision() (line 196 of core/modules/quickedit/quickedit.module).

I've been hitting the same issue, we should simply not assume the entity query will return a result. Using reset() will get around this because it doesn't care if the array is empty.

Bumping this to major, was tempted to even make it critical.

Is this issue about the asset library system passing a bad entity to quick-edit, or about quick-edit not being able to handle a bad entity?

They sound like separate issues...

@RajabNatshah - What's the advantage of #14 over #10? #10 is a lot less code and produces the same result.

Take this example:

php > $keys = array_keys([]); var_dump(reset($keys));
php shell code:1:
bool(false)

An empty array returned by the entity query passed into array_keys() then reset() will return FALSE, which is the result we want. We shouldn't need to explicitly return TRUE; or return FALSE;.

@RajabNatshah: I tested out your patch and it worked great. I'm running Drupal 8.4.0, Apache 2.4.17, and PHP 5.5.38 on Acquia Dev Desktop. I hope that it gets committed soon. Thanks.

Kat

Here's a test for this.

Interdiff from #10 because it's a more simple solution than #14.

Test only patch should fail.

I looked at _quickedit_entity_is_latest_revision() and since that was added we gained the ability to query for the latest revisions with entity queries, so tbh I would rewrite that method like this to make it more clear:

function _quickedit_entity_is_latest_revision(ContentEntityInterface $entity) {
  if (!$entity->getEntityType()->isRevisionable() || $entity->isNew()) {
    return TRUE;
  }

  $latest_revision = \Drupal::entityTypeManager()
    ->getStorage($entity->getEntityTypeId())
    ->getQuery()
    ->latestRevision()
    ->condition($entity->getEntityType()->getKey('id'), $entity->id())
    ->execute();

  return !empty($latest_revision) && $entity->getLoadedRevisionId() == key($latest_revision) ? TRUE : FALSE;
}

Also, I was wondering why the test added in #2903524: Don't add quickedit to displays where entities have a forward revision. does not fail (\Drupal\quickedit\Tests\QuickEditLoadingTest::testWithPendingRevision()). It seems to be doing exactly what's in the new test from #19..

so tbh I would rewrite that method like this to make it more clear:

👍

Also, I was wondering why the test added in […] does not fail […]

I think due to the root cause description given by @timmillwood in #11:

I've been hitting the same issue, we should simply not assume the entity query will return a result. Using reset() will get around this because it doesn't care if the array is empty.

The test added there never tests with zero revisions, I think.

Taking suggestion from #21.
Removing new test from #19.
Testing in existing webtest.

+++ b/core/modules/quickedit/src/Tests/QuickEditLoadingTest.php
@@ -330,6 +330,8 @@ public function testUserWithPermission() {
+    $this->drupalPostForm('node/add/article', ['title[0][value]' => 'foo'], t('Preview'));

We don't need to use t() in test code, unless we're actually testing the translation of that string :)

Fixing usage of t().

Ok, this looks great now!

This looks a lot like the other EFQ that @amateescu patched previously for revisions; I wonder if we should be abstracting it?

Let's add a comment to the test and maybe some more explicit assertions around it? Otherwise the coverage provided by the POST isn't very clear.

Hah, so preview allows us to test the case of "no revisions yet"? But there are no assertions. Interesting! Looking forward to seeing #23's test-only patch fail, because I don't yet understand how it will fail :D

@Wim Leers

It'll fail with:

Exception Notice     quickedit.module   196 _quickedit_entity_is_latest_revisio
    Undefined offset: 0
Exception Notice     quickedit.module   196 _quickedit_entity_is_latest_revisio
    Undefined offset: 0
Exception Notice     quickedit.module   196 _quickedit_entity_is_latest_revisio
    Undefined offset: 0
Exception Notice     quickedit.module   196 _quickedit_entity_is_latest_revisio
    Undefined offset: 0

Heh, I guess that works. But apparently @xjm independently raised the same concern. Testing failures through "undefined offset" notices isn't very explicit.

I think we can explicitly test this by repeating the same assertions checking absence? These:

    $this->assertNoRaw('data-quickedit-entity-id="node/' . $this->testNode->id() . '"');
    $this->assertNoRaw('data-quickedit-field-id="node/' . $this->testNode->id() . '/title/' . $this->testNode->language()->getId() . '/full"');

In #2924724: Add an API to create a new revision correctly handling multilingual pending revisions there is a patch to add a getLatestRevisionId() method which could be used here.

@wimleers - the issue is that when there are no revisions returned by the query it throws a notice, but no real error so the function returns false, which means the quickedit elements are still rendered. So unless warnings and errors are enabled, there's no way to know it failed. One option is to create an unsaved entity object and pass it to the function directly within the webtest.

This looks a lot like the other EFQ that @amateescu patched previously for revisions; I wonder if we should be abstracting it?

Like @timmillwood mentioned above, #2924724: Add an API to create a new revision correctly handling multilingual pending revisions is the issue which adds a "get latest revision ID" helper to its proper place, the entity storage. However, I think this issue is more advanced in the core review process so it can go in before that, so we need to update the @todo from _quickedit_entity_is_latest_revision to mentioned the issue linked above instead of #2784201: Track the latest_revision and latest_revision_translation_affected ID in the entity data table for revisionable entity types.

As for testing failures through "undefined offset" notices, #33 explains why we can not test this by asserting the return value of that function, so "undefined offset" notices is really the only thing we have to rely on :)

NW for changing the @todo and then it can go back to RTBC in my opinion.

#2865616: Add an option for EntityConverter to load the latest entity revision and fix all entity forms to use this option. also added a getLatestRevisionId(), but to EntityConverter. This too linked to #2784201: Track the latest_revision and latest_revision_translation_affected ID in the entity data table for revisionable entity types as Quickedit currently does.

@xjm, I've split the part from #2924724: Add an API to create a new revision correctly handling multilingual pending revisions which will actually allow us to remove this quickedit helper in 8.5.x into #2926483: Add API methods for determining whether an entity object is the latest (translation-affecting) revision

Given the new issue and the comments above, I think the existing patch is fine as a bugfix for 8.4.x.

Were we going to add that @todo though?

@xjm - there's already a @todo pointing to #2784201: Track the latest_revision and latest_revision_translation_affected ID in the entity data table for revisionable entity types similar to other places where we're getting the latest revision ID.

We could replace with #2924724: Add an API to create a new revision correctly handling multilingual pending revisions or #2926483: Add API methods for determining whether an entity object is the latest (translation-affecting) revision, but all three offer a way to improve or remove this function.

I'd rather have consistency of all places pointing to the same todo.

I'm sorry, I keep reading the patch and I don't see it...

Oh ah, there's already a @todo on the docblock for the function in HEAD, which doesn't appear in the diff. Sorry, that wasn't clear to me before. :)

Sorry for the confusion @xjm 😋

So the thing with the unexplained drupalPostForm() (that has no assertions but yet provides actual coverage) still bothers me; I feel like we should at least assert the expected content on preview.

For now I added a comment (in the attached), but it'd be great if we could assert something so that we're explictly testing a specific code path/bit of functionality. I agree we shouldn't assert the absence of a notice or anything, but we should also have a positive assertion to show that the preview post actually did anything.

Edit: clarity.

@xjm - here's a new patch which asserts the quick edit items do not appear on the preview page, also in the two cases the quick edit items don't appear I've added an assertion on the status code to make sure the page is actually loading with no "403" or "404" type responses, which would also prevent the quick edit items from appearing.

Helps if I upload the patch for the correct issue!

🍵

#45 tested and works well!

#45 looks good to go, just one nitpick about the comment:

+++ b/core/modules/quickedit/src/Tests/QuickEditLoadingTest.php
@@ -330,6 +330,12 @@ public function testUserWithPermission() {
+    // Verify that the preview is loaded correctly.

While the assertions are valid here to prove that quickedit is not showing on preview, we should change the comment on this block to explain that this is what we are proving.

Just went ahead and added the comment that I meant on #47 since Tim is busy with other things, in the hope to keep this moving forward =)

Setting this back to RTBC because I believe that #45 addressed #42, #46 looks to agree, and this is just a small comment clarifying the intention.

Thanks @Manuel Garcia!
+1 RTBC.

DrupalCI fail, back to RTBC.

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!