Returns empty key in views data for entities w/o revisions with described list builder.

Empty key contains 'operations' data.
Empty key

CommentFileSizeAuthor
#16 missed_revisionable_validation-2973137-11.patch2.72 KBtstoeckler
#11 missed_revisionable_validation-2973137-11.patch2.72 KBdavps
#11 missed_revisionable_validation-test-2973137-11.patch1.76 KBdavps
#10 missed_revisionable_validation--test-only--2973137-10.patch865 bytesefpapado
#8 missed_revisionable_validation-2973137-8.patch1.81 KBefpapado
#6 missed_revisionable_validation-2973137-6.patch1.85 KBefpapado
#2 missed_revisionable_validation-2973137-2.patch987 bytesdavps
empty-key.png13.39 KBdavps
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

david.pashaev created an issue. See original summary.

davps’s picture

davps CreditAttribution: davps as a volunteer and at DrupalJedi commented
Assigned: davps » Unassigned
Status: Active » Needs review
FileSize
missed_revisionable_validation-2973137-2.patch987 bytes
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Yes, this is a clear oversight. Patch looks perfect. We need some tests for this, though. That shouldn't be too hard, though, we already have EntityViewsDataTest::testGetViewsDataWithEntityOperations() for example, so I think we can either expand that or add a new similar method.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

efpapado’s picture

efpapado CreditAttribution: efpapado at Ramsalt Lab commented
FileSize
missed_revisionable_validation-2973137-6.patch1.85 KB

Would something like this be considered enough as a test?

efpapado’s picture

efpapado CreditAttribution: efpapado at Ramsalt Lab commented
Status: Needs work » Needs review
Issue tags: -Needs tests

Set to "needs review" and remove tag "needs tests"

efpapado’s picture

efpapado CreditAttribution: efpapado at Ramsalt Lab commented
FileSize
missed_revisionable_validation-2973137-8.patch1.81 KB

Actually this should be the correct one, if the test is acceptable.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented

Thanks @efpapado, the test looks great. To prove that it does what it says, can you upload a patch with just the test? That should then fail. That would be great.

efpapado’s picture

efpapado CreditAttribution: efpapado at Ramsalt Lab commented
FileSize
missed_revisionable_validation--test-only--2973137-10.patch865 bytes

OK

davps’s picture

davps CreditAttribution: davps as a volunteer and at DrupalJedi commented
FileSize
missed_revisionable_validation-test-2973137-11.patch1.76 KB
missed_revisionable_validation-2973137-11.patch2.72 KB

@efpapado Test #10 doesn't cover described case, because the entity used in test is revisionable. Test #10 passes for all entities with/without revisions.

Let's try this test and patch.

The last submitted patch, 11: missed_revisionable_validation-test-2973137-11.patch, failed testing. View results

efpapado’s picture

efpapado CreditAttribution: efpapado at Ramsalt Lab commented
Status: Needs review » Reviewed & tested by the community

Very nice! I think I can set it to RTBC now :)

The last submitted patch, 11: missed_revisionable_validation-test-2973137-11.patch, failed testing. View results

The last submitted patch, 11: missed_revisionable_validation-test-2973137-11.patch, failed testing. View results

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented
FileSize
missed_revisionable_validation-2973137-11.patch2.72 KB

Testbot is retesting the wrong patch, so here's a re-upload.

  • larowlan committed 3a87d23 on 8.8.x 
    Issue #2973137 by davps, efpapado, tstoeckler: EntityViewsData missed...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Version: 8.8.x-dev » 8.7.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 3a87d23 and pushed to 8.8.x. Thanks!

C/p to 8.7.x

  • larowlan committed 20bbd3a on 8.7.x 
    Issue #2973137 by davps, efpapado, tstoeckler: EntityViewsData missed...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.