Enforce the BundleConstraint "bundle" option to be always an array

If still a valid task wonder if the issue summary could be updated please.

I stumbled upon this from another issue.

This issue exists to address @effulgentsia's feedback at #1845546-121: Implement validation for the TypedData API → and @fago argued at #1845546-122: Implement validation for the TypedData API → it should be done in a follow-up. This is that follow-up.

But this issue predates us having solid strategies for making changes like that happen.

So … this must do a deprecation. Although AFAICT nothing in core is even using this anymore, with the exception of \Drupal\KernelTests\Core\Entity\BundleConstraintValidatorTest::assertValidation(). Not even \Drupal\Tests\Core\Plugin\Context\EntityContextDefinitionIsSatisfiedTest::providerTestIsSatisfiedByPassBundledEntity(). Let's find out!

The last submitted patch, 16: 1905620-16.patch, failed testing. View results →

We could totally continue with #16 here.

But as I wrote at #3382581-12: Add new `EntityBundleExists` constraint → :

1. I did a review in which I tried to RTBC this. And in doing so, I discovered the existence of \Drupal\Core\Entity\Plugin\Validation\Constraint\BundleConstraint (introduced in #1845546: Implement validation for the TypedData API → , with a follow-up that I just pushed forward after >10 years of silence: 📌 Enforce the bundle constraint option to be always an array Needs work ).

   I think we should document what the difference is between the (existing) Bundle and (new) BundleExists.

   AFAICT BundleConstraint should never have existed … it should just have used the Choice constraint instead?! It has existed since 2010, so it's not like it wasn't available 😅

2. …

… maybe the correct course of action here is to:

1. Deprecate Bundle altogether and switch to Choice
2. Alternatively, make Bundle an alias of Choice.

Note that Choice was explicitly added to Drupal core in ✨ Add a `langcode` data type to config schema Fixed — lots of things in Drupal core already use it!

Curious how it will affect existing code which is using Bundle in block annotations

Of the 2 options

1. Deprecate Bundle altogether and switch to Choice
2. Alternatively, make Bundle an alias of Choice.

Think 1 makes the most sense.

Making an alias even though it should cover backwards compatibility, think the idea of having 2 constraints around that do the same could be confusing to some.

📌 Add new `EntityBundleExists` constraint Fixed landed!

#20: I agree. Except that the validation error message for Bundle kinda makes more sense. Still, let's find out how much change it would cause if we actually removed this constraint.

In trying to do #21, I discovered I was wrong in #18. Choice works at the (property) value level. Bundle works at the entity object (EntityAdapter to be exact) level.

Change record created for just this tiny deprecation.

P.S.: @effulgentsia wrote #1845546-121: Implement validation for the TypedData API → .1 (which triggered this issue) exactly 11 years ago today.

Ran test-only feature and got a failure, see https://git.drupalcode.org/issue/drupal-1905620/-/jobs/723178

Updated the CR slightly to include an example, for visual people like myself.

Code looks good to me.

Thanks! The changes to the change record weren't quite accurate though, so fixed that. Please check it again and see if you like it? :)

I think the deprecation here should probably be for 12.0.0 now unless we really need to drop it in 11.x per discussion in 🌱 Defer disruptive 10.3 deprecations for removal until 12.0 Active .

Is this truly disruptive? It's a trivial change that modules can make today and it'd work fine all the way back to Drupal 8? 🤔

P.S.: following that issue now — very interesting!

I would like to move this back to RTBC and say this isn't truly disruptive, but I actually think it is, we don't know how custom entities look and for them they it would be a change.

Should be simple enough to just update the number in the deprecation

Should be simple enough to just update the number in the deprecation

👍 Let's do that.

Probably fine to self RTBC after that update.

It's a trivial change that modules can make today and it'd work fine all the way back to Drupal 8?

This is very true, but if they're minimally/under-maintained and already have Drupal 11 compatible releases (or if they don't need any other changes other than .info.yml so far), then it's disruptive for sites trying to update to Drupal 11 and finding out modules aren't compatible. You only need one or two modules not ready to delay a site update, and those modules only need one or two small deprecations preventing a fully 11.x-compatible release to be the ones that cause the delay - so lots of small core deprecations can add up.

For internal stuff where the bc we add is 'best effort' anyway like constructor changes, we definitely shouldn't apply this, especially because multiple changes to the same constructor gets cumulatively harder to account for. But public API breaks where the bc layer is easy, I think it's nicer to do this for the few months before the major release - if it's likely to cause pain, we can always decide the other way.

👍 Thanks for that extra context, @catch!

Rebased and updated the deprecation notice.

I don't get why we need to do this. DX is slightly nicer if developers can pass a string in the simple case or an array of strings otherwise. Given it's been over 10 years since this was proposed are there better things to work on?

Regarding #35 believe that should be answered by committer team?

But looking at the MR believe all feedback has been addressed.

There is one question remaining, which is whether to do this or not. See the remaining tasks in the issue summary.

Agreed with #35, I think we should mark this 'by design' and just support a single bundle or an array. There's no justification given in the issue summary or in @effulgentsia's original comment which is just one sentence of a larger review.