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

Rethink the namespace for Entity Bundle Classes #74

Closed
drubb opened this issue Jan 6, 2022 · 9 comments
Closed

Rethink the namespace for Entity Bundle Classes #74

drubb opened this issue Jan 6, 2022 · 9 comments

Comments

@drubb
Copy link

drubb commented Jan 6, 2022

For now, entity bundle classes generated by DCG 2 are placed in the namespace Drupal\mymodule\Entity\Bundle:

<?php

namespace Drupal\mymodule\Entity\Bundle;

use Drupal\node\Entity\Node;

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

However, most documentation and blog posts I've seen for now favor putting them directly in the 'Entity' namespace. Here's an excellent example: https://www.hashbangcode.com/article/drupal-9-entity-bundle-classes.

This is highly opinionated, but IMHO DCG should follow established best practices, so I'd suggest changing the namespace.

@weitzman
Copy link
Contributor

weitzman commented Jan 6, 2022

A bit of a stretch to appeal to "established best practices". Slight -1 from me. I like using a new namespace here. Its easier to delete the dir and regenerate classes.

@Chi-teck
Copy link
Owner

Chi-teck commented Jan 6, 2022

Entity Bundle Classes is still a pretty new thing so I doubt that best practices are already established on this. Personally I have no opinion about that. So I am ok to change this If considerable number of people request it.

@drubb
Copy link
Author

drubb commented Jan 6, 2022

Personally, I'm fine with the new namespace. But e.g. Drupal Core doesn't use it this way, e.g. in core/modules/system/tests/modules/entity_test_bundle_class/src/Entity:

<?php

namespace Drupal\entity_test_bundle_class\Entity;

use Drupal\Core\Entity\EntityStorageInterface;
use Drupal\entity_test\Entity\EntityTest;

/**
 * The bundle class for the bundle_class bundle of the entity_test entity.
 */
class EntityTestBundleClass extends EntityTest {
  ...
}

@drubb
Copy link
Author

drubb commented Jan 9, 2022

Currently doing some work on a pretty large codebase, I'd even introduce one more level for the namespace: the entity type!

So we'd end up with something like Drupal/mymodule/entity/Bundle/media or Drupal/mymodule/entity/Bundle/node

Really, it's totally opinionated - but what could be a reasonable default?

@weitzman
Copy link
Contributor

weitzman commented Feb 2, 2022

I think the entity type is a reasonable improvement. I'm dealing with a jumble of entity types in same folder right now.

@Chi-teck
Copy link
Owner

Chi-teck commented Feb 2, 2022

I wonder if "Bundle" suffix was a good idea. In Drupal bundles are configuration entities. So "ArticleBundle" sounds like a bundle configuration entity for nodes.

@ghost
Copy link

ghost commented Apr 26, 2022

I wonder if "Bundle" suffix was a good idea. In Drupal bundles are configuration entities. So "ArticleBundle" sounds like a bundle configuration entity for nodes.

I would now drop them in a Drupal/<modulename>/Entity/<EntityTypeName> namespace. Otherwise it would be confusing to drop multiple entity bundle classes of different entity types in the same folder. Then you would have to open the file before being able to check the entity type by checking the class extension.

If every which way the editor provided class jumping into this file I would say the namespace didn't matter. But since entity bundle classes provide public getFunctions to twig files it's still useful to provide a more descriptive namespace for the developer to find the right file.

I think as well that the Bundle suffix of the class name is not needed.

@DieterHolvoet
Copy link

I have been using entity bundle classes for about 4 years now, even before it landed in Drupal core. I always stuck to the Drupal/<modulename>/Entity/<EntityTypeName> convention.

@Chi-teck
Copy link
Owner

The generator has been updated in 3.x branch.

  1. "All" option has been removed. Symfony has fixed a bug with autocompletion for mutly-select questions. So
    that it's much easier now to select multiple bundles. That also made the implementation a bit simpler.
  2. Namespace for generated classes has been changed to Drupal/<modulename>/Entity/<EntityTypeName> as suggested above.
  3. Bundle suffix has been removed from suggested bundle class names.
  4. Added Base suffix to suggested name for base bundle class.
$ ./vendor/bin/dcg entity:bundle-class 

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

 Module machine name:
 ➤ example

 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

 Bundles, comma separated:
  [1] Article
  [2] Basic page
 ➤ 1,2

 Class for "Article" bundle [Article]:
 ➤ 

 Class for "Basic page" bundle [Page]:
 ➤ 

 Use a base class? [No]:
 ➤ Yes

 Base class [NodeBase]:
 ➤ 

 The following directories and files have been created or updated:
–––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––
 • example.module
 • src/Entity/Node/Article.php
 • src/Entity/Node/NodeBase.php
 • src/Entity/Node/Page.php

Thank you all for feedback.

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

No branches or pull requests

4 participants