Add new `EntityBundleExists` constraint

Looking great!

AFAICT this is ready once:

1. FieldConfig also adopts it 😇
2. one more edge case gets test coverage
3. a change record exists?

Drafted a change record: https://www.drupal.org/node/3384086 →

Looking good!

Only one concern: we can't just make \Drupal\Core\Config\TypedConfigManager::replaceVariable() public — doing that is a BC break. Posted a proposal on the MR! 😊

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. The IS does not mention the introduction of \Drupal\Core\Config\Schema\TypeResolver and why we do that.
3. Most of my remarks on the MR are about exposing the whole [%parent.something] syntax including the square brackets to users of validation constraints. We shouldn't do that IMHO — the resulting YAML is very tricky to misread as an array 😅 Also, we just don't need it!

This issue is very close IMHO! 🤩

This looks great, and was ready to rtbc apart from a few things in #12, I think all relevant questions from @Wim Leers have been answered, but leaving it for him to rtbc next week to be sure.

So very close, but this is still missing test coverage for invalid entityTypeId option values to ensure a good DX for developers adopting this validation constraint!

OK, I tried to add the requested test coverage. How's this look?

Back to rtbc, I really love how many of our tests are now more complete/explicit because of this new contraint.

As a sidenote, do we allow % in our schema's for things that are not replaceable? Because if we don't, we could probably throw an error and tell people something like: %paren found in resolved schema, there is a typo in your schema definition..

But that can 100% be a followup to this, what we have in this issue is already an improvement.

#18: WRT that validation: I said the same thing at #16 3 weeks ago. But I agree that can happen in a follow-up, especially because it's A) a pre-existing problem, B) in pre-existing code.

So, created the necessary follow-up: 📌 [PP-1] Validate inputs of config schema's TypeResolver: only allow %parent, %type and %key Postponed .

RTBC++

The follow-up created in #19 already has a patch. As soon as this is in, that one can be rerolled easily.

I read the issue summary, which was complete and easy to follow, and the comments. I didn't find any unanswered questions or other work to do. There are 2 unresolved comments from @Wim Leers in the MR but they have been addressed and @Wim Leers agreed this is RTBC.

The change record is up to date. Although I do wonder how people, for who English is a second language will interprets "There's a new validation constraint in town".

I did some hunting and found this quote from the Content style guide →

Avoid slang terms. Be sure any English-language idioms you use are common and well-known. This benefits all readers, but especially Drupal users with a first language other than English.

I am not sure how one is supposed to know that an idiom is common and well-know but we should probably remove that from the change record. It would help is someone changed that. I don't think the use of 'in town' makes the change record unintelligible so I am leaving this at RTBC.

Rewritten the first two lines based on #22:

We have introduced a new Validation Constraint with: EntityBundleExists.
This constraint takes the name of an entity bundle and confirms that it actually exists. It accepts only one (required) argument -- the entity type ID.

This needs a reroll, it no longer applies.

Merged 11.x back into this, back to rtbc.

It looks like I screwed up something with the merge I did. I will try to fix this.

The typeResolver that was introduced here (in TypedConfigManager), conflicts with other changes that been committed to 11.x in the meanwhile. I'm not sure how to best resolve this.

One of the breaking changes is in 🐛 Consistently use "dynamic type name" and "expression" instead of "variable value" in TypedConfigManager's terminology Fixed .

@borisson_: 🐛 Consistently use "dynamic type name" and "expression" instead of "variable value" in TypedConfigManager's terminology Fixed renamed TypedConfigManager::replaceName() and ::replaceVariable() to :: resolveDynamicTypeName() and ::resolveExpression(), respectively. But it kept them protected.

This issue was lifting those 2 methods out of that class and making them public.

I think we can still do the exact same thing: just make them static, move them out of TypedConfigManager and into TypeResolver, and make both public. That allows not breaking backwards compatibility.

Did the stuff Wim explained in #28. Since this was basically just fixing merge conflicts, I'm restoring RTBC on the assumption that tests will pass.

Thanks for fixing that @phenaproxima!

phpcs and phpstan violations.

TypeResolve and TypedConfigManager look solid in the MR though!

Actually, it's not a simple phpstan violation, it's because this entire else branch doesn't return anything, I think this means we are missing test coverage?

 else {
        // Get nested value and continue processing.
        if ($name == '%parent') {
          /** @var \Drupal\Core\Config\Schema\ArrayElement $parent */
          // Switch replacement values with values from the parent.
          $parent = $data['%parent'];
          $data = $parent->getValue();
          $data['%type'] = $parent->getDataDefinition()->getDataType();
          // The special %parent and %key values now need to point one level up.
          if ($new_parent = $parent->getParent()) {
            $data['%parent'] = $new_parent;
            $data['%key'] = $new_parent->getName();
          }
        }
        else {
          $data = $data[$name];
        }

I fixed the phpcs failures.

That code is moved directly from the pre-existing methods \Drupal\Core\Config\TypedConfigManager::resolveDynamicTypeName() and \Drupal\Core\Config\TypedConfigManager::resolveExpression(). I don't think any action is needed; this issue is only exposing those methods in a new class, but not changing them in any other way.

IMHO, if PHPStan is complaining about this, the correct approach is probably to shut it up here.

I agree with @phenaproxima, we can change the phpstan configuration later, or help it figure out what is going in here later, but I think this should just be the move.

I will try to pick this up after work

AFAICT this means we must update the PHPStan baseline?

I would adjust the baseline but I don't know how to do it. I tried editing the file manually but it doesn't seem to work properly. What am I missing?

Okay, well, after some wrasslin' I was finally able to get PHPStan to ignore that one error. However, once I got the regex correct, PHPStan had this to say:

     Error message "Method                                                      
     Drupal\Core\Config\Schema\TypeResolver::resolveExpression() should return  
     string but return statement is missing." cannot be ignored, use            
     excludePaths instead.     

Well...shit. So I ended up just ignoring that one path entirely, and PHPStan worked for me locally with no errors.

Back to work for the test failures.

This was rtbc in #15 and #18 already, back to rtbc now that this is green again.

Left some comments on the MR

AFAICT everything was thoroughly addressed. I'm not a big fan of changing the decade-old logic that we're just lifting from elsewhere and making accessible, but it sure is more readable, and it was requested by core committer @larowlan so 👍 Two birds one stone!

Clarifying that this blocks 📌 [PP-1] Convert field_storage_config and field_config's form validation logic to validation constraints Postponed , which is . It also blocks 🌱 [meta] Add constraints to all config entity types Active . This priority seems justified IMO.

Issue credits

Can we get https://www.drupal.org/node/3408266 → updated to reflect the code, the method names haven't been renamed, they've been moved to a new TypeResolver class.

Fine to self RTBC, will keep an eye out for the update.

Also left a comment about phpstan changes

Tried to answer your questions, @larowlan, and updated the change record.

Added a change to fix the missing return type, please put back to RTBC if you're happy

Yup, I'm a lot happier without needing to mess with PHPStan's configuration! Thanks @larowlan.

Committed to 11.x
Published both change records

larowlan → closed merge request !4636

Thank you, @larowlan!

Unblocked & rerolled:

1. #3392903-9: Validate inputs of TypeResolver::resolveExpression(): only allow %parent, %type and %key →
2. #3324140-35: Convert field_storage_config and field_config's form validation logic to validation constraints →
3. #1905620-23: Enforce the BundleConstraint "bundle" option to be always an array →

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