Problem/Motivation

@penyaskito found this on #2976334: Allow Custom blocks to be set as non-reusable adding access restriction based on where it was used.

This made one of my upgrade tests fail:

1) Drupal\Tests\lingotek\Functional\Update\LingotekTargetStatusFormatterUpdate8209Test::testUpgrade
Schema key views.view.block_content:display.default.display_options.filters.reusable.group failed with: variable type is string but applied schema class is Drupal\Core\TypedData\Plugin\DataType\IntegerData

Found out the culprit here. The attached patch fixed it. Maybe we are missing some coverage in the upgrade tests?

I am not sure why LingotekTargetStatusFormatterUpdate8209Test failed but \Drupal\Tests\block_content\Functional\Update\BlockContentReusableUpdateTest did not.

I don't think affects the functionality the update still works so that is why I am creating a follow up.

Proposed resolution

  1. Change to an int
  2. Figure out why our update test didn't catch this. Presumably all update test would have broken in the same way.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#15 2988435-15-with-int.patch1.03 KBeffulgentsia
#15 2988435-15-with-int-like-string.patch429 byteseffulgentsia
#13 2988435-13-should-fail.patch1.03 KBeffulgentsia
#8 2988435-should-fail.patch624 byteseffulgentsia
#6 2988435-fix-assert-same.patch5.48 KBtedbow
#6 2988435-no_fix_equals_passes.patch5.54 KBtedbow
#4 2976334-upgrade-tests-115.patch621 bytestedbow
2976334-upgrade-tests-115.patch621 bytestedbow
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tedbow created an issue. See original summary.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented

Test run!

tedbow credited penyaskito.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
FileSize
2976334-upgrade-tests-115.patch621 bytes

This is @penyaskito's patch. Crediting

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented
Priority: Normal » Critical

Raising to Critical, since it's an upgrade path error.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
FileSize
2988435-no_fix_equals_passes.patch5.54 KB
2988435-fix-assert-same.patch5.48 KB

Ok here are couple patches

both modify core/drupalci.yml to only run "Drupal\Tests\block_content\Functional\Update\BlockContentReusableUpdateTest"

2988435-no_fix_equals_passes

  1. Doesn't have the fix.
  2. Used assertEquals to test that views actually do get updated now. I also show that with assertEquals 1 or '1' is the same and will pass

2988435-fix-assert-same

Has the fix
Uses assertSame which will check types

Bigger problem

I think 2988435-no_fix_equals_passes proves that what is committed now actually does save the filter and BlockContentReusableUpdateTest tests that filter is respected after the update.

I think for Views the '1' or 1 will work the same.

But are update tests aren't actually throwing exceptions when we update config and don't get the schema exactly right.

I have created a locally update test that simply does


/**
   * {@inheritdoc}
   */
  protected function setDatabaseDumpFiles() {
    $this->databaseDumpFiles = [
      '/Users/ted.bowman/Sites/www/d8/modules/contrib/lingotek/tests/fixtures/update/drupal-8.lingotek.standard.pre8209.php.gz'
     //'/Users/ted.bowman/Sites/www/d8/core/modules/system/tests/fixtures/update/drupal-8.bare.standard.php.gz',
      //__DIR__ . '/../../../../../system/tests/fixtures/update/drupal-8.4.0.bare.standard.php.gz',
      //__DIR__ . '/../../../../../system/tests/fixtures/update/drupal-8.2.0.bare.standard_with_entity_test_revlog_enabled.php.gz',
    ];
  }

/**
   * Tests running updates.
   */
  public function testUpdates() {
    $this->runUpdates();
  }

you can see here that I was trying different fixture files. None of the fixtures I tested from core throw the schema error but drupal-8.lingotek.standard.pre8209.php.gz did.

I am not sure what the difference is.

We should probably open a general schema testing issue for this to prove it is the case.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented

But are update tests aren't actually throwing exceptions when we update config and don't get the schema exactly right.

I don't know. But another factor here is that '1' does not violate the config schema of integer, because PrimitiveTypeConstraintValidator::validate() merely does a filter_var($value, FILTER_VALIDATE_INT) check, which passes for integer-like strings.

So potentially this bug isn't Critical after-all, but still worth fixing and figuring out how to make the test coverage more robust.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented
FileSize
2988435-should-fail.patch624 bytes

This should fail if our update tests are validating config schema.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented

By the way, #2414951: Validate configuration schema before importing configuration would make it easier to test config for schema validity via the UI, but alas, we don't have that yet.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented

We should probably open a general schema testing issue for this to prove it is the case.

Hm, yeah, I think #8 passing is a concern. I don't know yet if there's something specific to this issue that's causing it to pass (e.g., the '1a' is being converted to a 1 at some point in the update process) or whether the assertConfigSchema() in runUpdates() is failing to catch schema violations. If the latter, then a new issue makes sense. If the former, we should probably figure it out in this issue.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito at Lingotek commented

I created drupal-8.lingotek.standard.pre8209.php.gz with 8.5.x. Maybe that's a thing.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented

I think #11 is correct. I think what's likely happening is that the test is running some other post_update function after block_content_post_update_add_views_reusable_filter(), and that later running one is cleaning up any schema mismatch created by the former one. I suspect the culprit is the views_post_update_filter_placeholder_text() function added in #2917594: Add support for HTML5 placeholder in views exposed filters, since that's doing a $view->save() without either using ConfigEntityUpdater or invoking $view->trustData() directly. Also, that issue was committed to 8.5.x, but not to 8.4.x, which would explain the difference between a test run starting from an 8.5 database vs. an 8.4 one.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented
FileSize
2988435-13-should-fail.patch1.03 KB

Testing #12's hypothesis.

Status: Needs review » Needs work

The last submitted patch, 13: 2988435-13-should-fail.patch, failed testing. View results

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented
Status: Needs work » Needs review
FileSize
2988435-15-with-int-like-string.patch429 bytes
2988435-15-with-int.patch1.03 KB

Great! Ok, let's try these now then.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Needs review » Reviewed & tested by the community

That's the nice thing about ConfigEntityUpdater, you don't need to remember the magic of trustData() yourself.
Might be worth a follow-up to go back through old update functions to switch them to that, just to establish the patten for those seeking to mimic an update function.

The last submitted patch, 15: 2988435-15-with-int-like-string.patch, failed testing. View results

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented

Adding reviewer credit.

  • effulgentsia committed 7e8c4db on 8.7.x 
    Issue #2988435 by effulgentsia, tedbow, penyaskito, tim.plunkett:...

  • effulgentsia committed 737c838 on 8.6.x 
    Issue #2988435 by effulgentsia, tedbow, penyaskito, tim.plunkett:...
effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented
Version: 8.7.x-dev » 8.6.x-dev
Status: Reviewed & tested by the community » Fixed

I pushed @penyaskito's original patch (#4) to 8.7.x and 8.6.x.

I'll open a new issue to discuss changing already released post_update functions, like the one in #15.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented
Related issues: +#2989256: Old post_update functions that ->save() config entities without calling ->trustData() first mask errors in other post_update functions

Here's that new issue: #2989256: Old post_update functions that ->save() config entities without calling ->trustData() first mask errors in other post_update functions.

Status: Fixed » Closed (fixed)

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