Problem/Motivation

The module page currently says:

Configure settings (optional)
Go to: /admin/config/content/canvas-entity-reference

Select which taxonomy vocabularies are available for entity reference props. Leave unchecked to allow all.
Enable or disable automatic creation of new terms.
Select widget

Now that the module allows using all entity types and most of the configuration happens in the schema, I think it would make sense to remove that setting and instead define the allowed bundles in schema, e.g.:
x-entity-type-bundles: tags, folders

To allow any (new) bundles, we maybe need a placeholder logic like:
x-entity-type-bundles: NULL
or NOT defining x-entity-type-bundles at all?

What do you think?

The other option would be having a settings page listing all entity types and their bundles in Drupal to select the allowed ones in UI, but I think that would be less flexible and even more complicated?

This will need a new major release (2.x) - I'd vote to start with an alpha for now, following semantic versioning.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Comments

anybody created an issue. See original summary.

zeeshan_khan’s picture

zeeshan_khan commented

You are 100% right I will see what flexibilities I have with Canvas if I find it working will try to implement it in next semantic release as you suggested.
The only reason I didn't use the semantic release approach earlier was - I work for a client they uses only standard releases like 1.0.0 they don't allow semantic release like alpha beta etc.. and I needed the functionality for term reference only but in my mind I had the bigger picture to make it work for all entity types.
Hope that make sense now! :D

zeeshan_khan’s picture

zeeshan_khan commented

After looking into this, the approach is feasible. The plan is to add x-entity-type-bundles support as an additive change in a 1.1.x release, keeping the existing settings page for now. The settings page will be deprecated and removed in 2.x, which will be the breaking release. Will update this issue once work starts.

Feel Free to correct or suggest anything!

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

@zeeshan_khan that sounds perfect!

What do you think about the "allow all bundles" fallback? Simply support all if none defined?

zeeshan_khan’s picture

zeeshan_khan commented

(Note: All changes live in dev release)

Implemented x-entity-type-bundles as an additive change. Component authors can now declare bundle restrictions directly in the component YAML:

category:
type: ['string', 'null']
x-entity-type: taxonomy_term
x-entity-type-bundles: 'tags'

article_ref:
type: ['string', 'null']
x-entity-type: node
x-entity-type-bundles: 'article'

Works for all entity types. For taxonomy terms, the schema annotation takes precedence over the global vocabulary setting on the settings page. Omitting the annotation allows all bundles. The settings page is kept as-is for this release and will be deprecated in a future major version.

A test-bundle-filter component is included in the module for manual verification.

zeeshan_khan’s picture

zeeshan_khan commented
Status: Active » Needs review
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Am I missing something or is there no Kernel test for the x-entity-type-bundles property (with one or multiple bundles provided yet?

I saw https://git.drupalcode.org/project/canvas_entity_reference/-/blob/1.0.x/... but no test for defined bundles?

For documentation, I think the full example from PHPDoc (https://git.drupalcode.org/project/canvas_entity_reference/-/blob/1.0.x/...) should also be given in the README.md?

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Maybe you could test against test-bundle-filter component?

  • zeeshan_khan committed 77144f78 on issue/3589499-x-entity-type-bundles-tests-and-docs 
    Issue #3589499 by zeeshan_khan: Add kernel tests for x-entity-type-...
zeeshan_khan’s picture

zeeshan_khan commented

MR https://git.drupalcode.org/project/canvas_entity_reference/-/merge_reque...

Added 7 kernel tests to EntityReferenceShapeMatcherTest covering all the scenarios raised:

1. Single bundle restriction on taxonomy_term
2. Multiple bundles on taxonomy_term
3. Invalid bundle silently falls back to allowing all bundles
4. Omitted annotation allows all bundles
5. Bundle restriction on a non-taxonomy entity type (node)
6. Bundle restriction on array-type items
7. Schema-level x-entity-type-bundles takes precedence over the global config setting

Also expanded the README x-entity-type-bundles annotation reference with array-type examples (single vocabulary, multi-bundle array items) and a note explaining the invalid-bundle fallback behaviour.

  • zeeshan_khan committed 239addbf on 1.0.x 
    Issue #3589499 by zeeshan_khan: Add kernel tests for x-entity-type-...
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Needs review » Needs work

@zeeshan_khan thank you very very much for all this awesome work!

Just one more (probably dumb) question:
Why is x-entity-type-bundles: 'article, page' a comma-separated string, while type: ['string', 'null'] is an array?

# Array prop — multiple nodes from multiple bundles
related:
  type: array
  items:
    type: ['string', 'null']
    x-entity-type: node
    x-entity-type-bundles: 'article, page'

I'd expect x-entity-type-bundles to also be an array, which may also be empty or have just one value. I think that would be cleaner? Or is that simply not possible because in schema definitions?

Sorry as written above, this may be a dumb question because I don't have much experience with these schema values yet!

And another (hopefully useful) suggestion:
I think it would make to also add such a test and expect for the logger entries, where they should be?

  /**
   * @covers ::apply
   */
  public function testApplyBundleFilterInvalidBundleDroppedFallsBackToOthers(): void {
    Vocabulary::create(['vid' => 'tags', 'name' => 'Tags'])->save();

    $candidate = $this->makeCandidate([
      'type' => 'string',
      'x-entity-type' => 'taxonomy_term',
      'x-entity-type-bundles' => 'nonexistent_vocab, categories',
    ]);
    $this->matcher->apply($candidate);

    // Expect non existing bundles to be ignored, but keep the existing ones:
    $this->assertSame(
      ['categories' => 'categories'],
      $candidate->fieldInstanceSettings['handler_settings']['target_bundles']
    );

    // @todo Add an expect for the log entry here? (and in other tests like this)
  }

Thank you very much!!

  • zeeshan_khan committed 2f47d771 on issue/3589499-x-entity-type-bundles-tests-and-docs 
    Issue #3589499 by zeeshan_khan: Support YAML array format for x-entity-...

  • zeeshan_khan committed 239addbf on issue/3589499-x-entity-type-bundles-tests-and-docs 
    Issue #3589499 by zeeshan_khan: Add kernel tests for x-entity-type-...

  • zeeshan_khan committed 239addbf on issue/3588752-readme-contrib-widget-example 
    Issue #3589499 by zeeshan_khan: Add kernel tests for x-entity-type-...
zeeshan_khan’s picture

zeeshan_khan commented
Status: Needs work » Needs review

Thanks for the feedback and the suggested test!

PR - https://git.drupalcode.org/project/canvas_entity_reference/-/merge_reque...

On the array format — good catch. The string format was chosen for brevity in examples, but there is no reason to restrict it. The annotation now accepts both a comma-separated string and a native YAML array, so both of these are equivalent:

x-entity-type-bundles: 'tags, categories'
x-entity-type-bundles: ['tags', 'categories']

On the suggested test — the test in the comment had a logic error: it created only the tags vocabulary but asserted that categories would be returned, when categories does not exist on the site and would be dropped. The correct scenario for that assertion requires creating categories first.

What was actually missing was a test for partial validity — a bundle list where some entries are valid and some are not. That has been added: testApplyBundleFilterPartiallyInvalidKeepsValidBundles creates tags, passes 'tags, nonexistent_vocab', and asserts that only ['tags' => 'tags'] is kept.

Also added testApplyBundleFilterAcceptsYamlArrayFormat to cover the new array input support.

  • zeeshan_khan committed 485dbe42 on 1.0.x 
    Issue #3589499 by zeeshan_khan: Add kernel tests for x-entity-type-...

  • zeeshan_khan committed 485dbe42 on issue/3588752-readme-contrib-widget-example 
    Issue #3589499 by zeeshan_khan: Add kernel tests for x-entity-type-...
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

On the array format — good catch. The string format was chosen for brevity in examples, but there is no reason to restrict it. The annotation now accepts both a comma-separated string and a native YAML array, so both of these are equivalent:

x-entity-type-bundles: 'tags, categories'
x-entity-type-bundles: ['tags', 'categories']

My personal vote is always being strict about this and only allow an array with 0-n values. I think not being strict broadens the risk for issues. But that's my personal oppinion.

On the suggested test — the test in the comment had a logic error: it created only the tags vocabulary but asserted that categories would be returned, when categories does not exist on the site and would be dropped. The correct scenario for that assertion requires creating categories first.

SORRY yes that was a stupid mistake I made here in the textarea -.- Of course I should have done it the other way around, just like you did, thank you for the fix!

zeeshan_khan’s picture

zeeshan_khan commented

Thanks for the honest note on the test mistake, happens to all of us!

On strict array-only: it's a fair point and I appreciate the suggestion. That said, I'd like to keep both formats for now. The string form is already in the documentation and examples, so removing it would silently break existing component YAMLs for anyone who has already adopted it. Supporting both is also a common Drupal convention — for a single bundle, x-entity-type-bundles: 'article' reads more naturally than x-entity-type-bundles: ['article']. The normalisation in resolveBundlesFromSchema() handles both cleanly, so there is no real added complexity or risk.

zeeshan_khan’s picture

zeeshan_khan commented

@julian - is it good to go? can you mark this to RTBC?
Thanks

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Needs review » Reviewed & tested by the community

Thanks @zeeshan_khan yes that's your final decision of course :) RTBC.

So you're good to to tagging a new release.

zeeshan_khan’s picture

zeeshan_khan commented
Status: Reviewed & tested by the community » Fixed

ThankYou

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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