Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[generate:entity:content] Add 'view_builder' to config entity annotation template. Fix #3129. #3311

Merged
merged 1 commit into from
May 26, 2017

Conversation

jall
Copy link
Contributor

@jall jall commented May 14, 2017

This is a patch for #3129.

This allows content entities with bundles to be exposed through the core REST API without throwing errors of the form:

Route entity.{{entityBundleType}}.canonical does not exist.

Steps to reproduce & fix manually have been well described by @tahirm in the linked issue, so I won't re-explain them here.

This patch adds a default 'view_builder' to all config entity definitions. It uses the Core Drupal\Core\Entity\EntityViewBuilder & doesn't bother defining a custom extension class in the same way as, for example, the 'list_builder': Drupal\{{ module }}\{{ entity_class }}ListBuilder as I haven't come across a use case for this level of flexibility yet.

I checked Core to see where it extends EntityViewBuilder rather than using it directly and found of the >30 Drupal Core config entities, only 2 have 'view_builder' defined:

  • \Drupal\tour\Entity\Tour => Drupal\tour\TourViewBuilder
  • \Drupal\block\Entity\Block => Drupal\block\BlockViewBuilder

These both appear to be bundle config entities, which fits with the use case being fixed here.

I was concerned that adding 'view_builder' unconditionally to entity.php.twig might be too far reaching, as it doesn't look like a config entity needs it defined unless they specify a canonical route. However, it appears that the only usage of this template is for adding content entity's bundle classes in src/Generate/EntityConfigGenerator, so I believe this patch will only affect what it should.

This allows content entities with bundles to be exposed
through the core REST API without throwing errors of the
form:
"Route entity.{{entityType}}.canonical does not exist."
@jmolivas jmolivas modified the milestone: RC-20 May 15, 2017
@jmolivas jmolivas changed the title [generate:entity:content] Add 'view_builder' to config entity annotation template [generate:entity:content] Add 'view_builder' to config entity annotation template. Fix #3129. May 26, 2017
@jmolivas jmolivas merged commit 2499a1c into hechoendrupal:master May 26, 2017
@jmolivas
Copy link
Member

@jall Thanks for your contribution, your PR was merged. This will be included on the next release.

@davereid
Copy link

I think this should be reverted. This line is not necessary on the majority of config entities.

@jall
Copy link
Contributor Author

jall commented Mar 29, 2018

@davereid I can see why a view_builder would be dead code for many config entities which are not exposed/rendered, but does it introduce any performance impact or have other negative impacts other than cleanliness?

I raised this PR because I was generating a lot of bundle-able entities and exposing them as REST resources, & it was quite irritating having them fail by default before this patch.

I know very little about the bigger picture entity system but a lot of the other generated entity code from Drupal console seems be fairly verbose boilerplate & possibly unnecessary for many use cases, so I didn't think this would be any different.

@jmolivas
Copy link
Member

We can probably pass some flags to decide which code to generate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants