Add validation constraints to block_content.type.*

This is a bit tricky, because it is actually currently not a correct date type.

This is the current schema:

    revision:
      type: integer
      label: 'Create new revision'

The same thing happens on node types, and that looks a bit different:

    new_revision:
      type: boolean
      label: 'Whether a new revision should be created by default

As far as I understand it, these actually do the same thing, and they are validatable, because they are booleans.

We can't just make it the boolean type, because it might be the same in the database (0/1), but in the schema it is written as true/false instead of the integer, so I think this one needs an upgrade path as well?

I don't think we should update the key to new_revision, because while that would be more consistent; and easier to understand, that will probably be more difficult to land? I'm curious to see what the block content maintainers think.

I'll add a Merge request that changes the data type and let's see what breaks.

Seems to have some test failures in the pipeline

I think I fixed all the broken tests, also merged 11.x back into this.

Ran locally and update hook ran fine. And change to boolean makes sense. LGTM.

@sourabhjain You applied my suggestion locally and then pushed it. But you added a trailing blank line, which is now causing a phpcs failure. More tests will fail too. Can you address those things too? 🙏😊

Yes @wim-leers. I saw your suggestion on PR and after that applied it manually on local and pushed it.
And sorry for trailing blank line issue. I have pushed it again. Thanks

Good! 👍

Now there are 114 kernel test failures, 77 functional test failures and 17 functional JS test failures. Can you push those forward too? 🙏

I apologize for any inconvenience, @wim-leers. I misunderstood the suggestion, thinking it was a provided task that anyone could work on. I will be more careful in the future and ensure that such misunderstandings don't happen again. Rest assured, everyone at Valuebound will be mindful of this.

Causing test failures is fine of course! 😄 I make a hundred mistakes every day. You're right that anybody can work on this. But … only applying a suggestion that takes a single click to apply is not quite helpful. A suggestion is meant to make it easier for whichever person is working on the merge request, to speed up addressing the feedback in a review. IOW: blindly applying suggestions is not helpful; it causes confusion about the state of a merge request.

Looking forward to our next interaction! 😊

Addressed most of Wim's suggestions. We will need to:
1. Fix all the tests, the test failures are caused by the configuration not matching the schema, so we will need to add `'revision' => FALSE,` in a whole bunch of places.
2. Add an upgrade path test, I haven't done that before and I'm not sure where I'll find the time to learn how to write one.

Both of these can definitely be picked up by someone else.

I started changed some of the tests to improve the state of the pr, but I think I need to make a lot of changes like this in tests:

@@ -37,6 +37,7 @@ public function testReusableBlocksOnlyAreDerived() {
       'id' => 'spiffy',
       'label' => 'Mucho spiffy',
       'description' => "Provides a block type that increases your site's spiffiness by up to 11%",
+      'revision' => FALSE,
     ]);

This doesn't make sense to me, is there a better way to do this?

Self-assigning for update path tests.

Back to you!

Seems to have test failures.

Failures a-fixed!

Sweet! Seems validation for revision is good and didn’t break anything. Excited to see these improvements

Found 2 actual bugs, 1 question.

I'm not sure if i can rtbc the latest changes since I've worked on this issue a lot as well, but I agree with @phenaproxima that this is committable as is.

The last commit (aka my suggestion) introduced a small regression, sorry:

Exception: Deprecated function: trim(): Passing null to parameter #1 ($string) of type string is deprecated

Easy fix though: add ?? '' ← applied this trivial change 👍

P.S.: one of my remarks on the MR was indeed wrong, as @phenaproxima pointed out. Opened an issue for that (which does not block this one): 📌 Improve NotBlank DX in config (schema) validation Needs review 👍

Committed to 11.x

Thanks

larowlan → closed merge request !5170

Yay, this brought us to 0.9 → 1.1% of all types being fully validatable, and bumped the Standard install profile from 4.4 to 4.9%! https://project.pages.drupalcode.org/config_inspector/ 🚀

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