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

Add bundle classes generator #4903

Closed
wants to merge 6 commits into from
Closed

Add bundle classes generator #4903

wants to merge 6 commits into from

Conversation

weitzman
Copy link
Member

@weitzman weitzman commented Nov 17, 2021

  • Generates bundle classes for one or more entity types (e.g. node, media).
  • Generates an abstract base class that bundle classes extend
  • Generates a hook_entity_bundle_info_alter implementation

Demo

  • Below is what node,media generation looks like, on the CLI
  • This gist shows the generated classes
  • Devel screenshot below showing that Drupal is handing out Page class instead of Node
$ ./drush gen entity:bundle-classes

 Welcome to entity:bundle-classes generator!
–––––––––––––––––––––––––––

 Module machine name [sut]:
 ➤ drush_empty_module

 Entity type(s). Use comma to delimit. [node]:
  [ 0] block_content
  [ 1] comment
  [ 2] contact_message
  [ 3] file
  [ 4] media
  [ 5] menu_link_content
  [ 6] node
  [ 7] path_alias
  [ 8] shortcut
  [ 9] taxonomy_term
  [10] user
 ➤ 6,4
[warning] Run `drush cache:rebuild` so the bundle classes are recognized.

 The following directories and files have been created or updated:
–––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––
 • /var/www/html/sut/modules/unish/drush_empty_module/drush_empty_module.module
 • /var/www/html/sut/modules/unish/drush_empty_module/src/Bundle/media/Audio.php
 • /var/www/html/sut/modules/unish/drush_empty_module/src/Bundle/media/Document.php
 • /var/www/html/sut/modules/unish/drush_empty_module/src/Bundle/media/Image.php
 • /var/www/html/sut/modules/unish/drush_empty_module/src/Bundle/media/MediaBundleBase.php
 • /var/www/html/sut/modules/unish/drush_empty_module/src/Bundle/media/RemoteVideo.php
 • /var/www/html/sut/modules/unish/drush_empty_module/src/Bundle/media/Video.php
 • /var/www/html/sut/modules/unish/drush_empty_module/src/Bundle/node/Article.php
 • /var/www/html/sut/modules/unish/drush_empty_module/src/Bundle/node/NodeBundleBase.php
 • /var/www/html/sut/modules/unish/drush_empty_module/src/Bundle/node/Page.php

image

Todo

function drush_empty_module_entity_bundle_info_alter(array &$bundles): void {
{% for id in entity_type_ids %}
{% for bundle in infos[id]|keys %}
$bundles['{{ id }}']['{{ bundle }}']['class'] = \Drupal\{{ machine_name }}\Bundle\{{ id }}\{{ bundle|camelize }}::class;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default PHPCS lints will want this moved into a use statement. Given that it can also automatically fix the lint, I'd say no change is warranted here.

@Chi-teck
Copy link
Collaborator

Chi-teck commented Nov 17, 2021

@weitzman I think this command should generate classes for just one entity type at a time and I would let user to specify a bundle.

@Chi-teck
Copy link
Collaborator

Also I think generating abstract class should be optional. For instance, for custom entities there is no point to have a base bundle class as everything can be implemented in entity class.

Copy link
Collaborator

@Chi-teck Chi-teck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/Bundle

How about src/Entity or src/Entity/Bundle? Though, I am not sure which path is better.

@Chi-teck
Copy link
Collaborator

Chi-teck commented Nov 17, 2021

Aren't bundle classes only applicable to content entity types?

@weitzman
Copy link
Member Author

  1. Oh right, I will filter to just content entities
  2. As for generating just one entity type at a time, how would you handle the hook, which needs info from all entities and bundles. You wouldnt want to remove whats already there. Would you only write that hook if it already exists? Seems a bit weird for the generator to work the first time you ran it but not for subsequent entity types.

@weitzman
Copy link
Member Author

weitzman commented Nov 17, 2021

I am now only showing content entities (OP is updated), and I added the option to skip base class.

Update: I am now using src/Entity/Bundle namespace instead of src/Bundle

@Chi-teck
Copy link
Collaborator

Chi-teck commented Nov 18, 2021

how would you handle the hook, which needs info from all entities and bundles. You wouldnt want to remove whats already there. Would you only write that hook if it already exists? Seems a bit weird for the generator to work the first time you ran it but not for subsequent entity types.

I know that's tricky. The current approach is just generate a second instance of the hook and let user to merge it manually. That's how it works in template generator. I am trying to avoid parsing PHP code to keep the things simple. So far hooks are
the only place where it's really needed. Note that generating classes for multiple entity types will not eleminate the need to run the generator again later because over time new entity types and bundles may be added on a site.

@weitzman
Copy link
Member Author

Double printing the alter hook sounds reasonable to me ... I think I've responded to all your feedback except for allowing for bundle(s) to be specified. Should be simple.

@Chi-teck
Copy link
Collaborator

Chi-teck commented Nov 18, 2021

I'm OK with this moving to DCG if you think thats better.

I've just pushed it to DCG 2.x.
https://github.com/Chi-teck/drupal-code-generator/blob/master/src/Command/EntityBundleClass.php

Output

$ vendor/bin/dcg entity-bundle-class --dry-run

 Welcome to entity-bundle-class generator!
–––––––––––––––––––––––––––––––––––––––––––

 Module machine name [bar]:
 ➤ 

 Entity type:
  [ 1] Custom block
  [ 2] Comment
  [ 3] Contact message
  [ 4] File
  [ 5] Custom menu link
  [ 6] Content
  [ 7] URL alias
  [ 8] Shortcut link
  [ 9] Taxonomy term
  [10] User
 ➤ Content

 Bundle:
  [1] Article
  [2] Basic page
 ➤ Article

 Class [ArticleBundle]:
 ➤ 

 Would you like to generate a base class? [No]:
 ➤ Yes

 Base class [NodeBundle]:
 ➤ 

 src/Entity/Bundle/NodeBundle.php
––––––––––––––––––––––––––––––––––
<?php

namespace Drupal\bar\Entity\Bundle;

use Drupal\node\Entity\Node;

/**
 * A base bundle class for node entities.
 */
abstract class NodeBundle extends Node {

}


 src/Entity/Bundle/ArticleBundle.php
–––––––––––––––––––––––––––––––––––––
<?php

namespace Drupal\bar\Entity\Bundle;

/**
 * A bundle class for node entities.
 */
class ArticleBundle extends NodeBundle {

}


 bar.module
––––––––––––
<?php

/**
 * @file
 * Primary module hooks for Bar module.
 */

/**
 * Implements hook_entity_bundle_info_alter().
 */
function bar_entity_bundle_info_alter(array &$bundles): void {
  $bundles['node']['article']['class'] = \Drupal\bar\Entity\Bundle\ArticleBundle::class;
}

Feel free to submit bug reports and improvements. Thank you!

@Chi-teck Chi-teck closed this Nov 18, 2021
@weitzman weitzman deleted the bundle-classes branch November 18, 2021 13:46
/**
* A bundle class for {{ entity_type_id }} entities.
*/
class {{ bundle_class }} extends {{ parent_class|split('\\')|last }} {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, we should encourage bundle classes to implement bundle interfaces. A Page bundle class could implement PageInterface. This is very useful, as a bundle class can implement multiple interfaces:

interface PageInterface {
  public function getContent();
}

interface ArchivableInterface {
  public function getArchivedDate();
}

class Page implements PageInterface, ArchivableInterface {
}

also this allows to do some more semantically checks:

if ($node instanceof PageInterface) {...}

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

Successfully merging this pull request may close these issues.

None yet

4 participants