In #2204697-41: Move getConfigPrefix() to ConfigEntityTypeInterface outline several ways the the config_prefix and ConfigEntityTypeInterface::getConfigPrefix() documentation could be improved:

  1. +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
    @@ -102,7 +103,7 @@ public function __construct(EntityManagerInterface $entity_manager, ConfigFactor
       public function getEntityTypeIdByName($name) {
         $entities = array_filter($this->entityManager->getDefinitions(), function (EntityTypeInterface $entity_type) use ($name) {
    -      return ($config_prefix = $entity_type->getConfigPrefix()) && strpos($name, $config_prefix . '.') === 0;
    +      return ($entity_type instanceof ConfigEntityTypeInterface && $config_prefix = $entity_type->getConfigPrefix()) && strpos($name, $config_prefix . '.') === 0;
         });
         return key($entities);
       }
    

    Hmm. So if the entity type is an instance of ConfigEntityTypeInterface, but getConfigPrefix() returns something that is false, or the name doesn't begin with the config prefix expected... then what happens?

    This isn't introduced by this patch, but it seems like now this is a set of things that would mean the entity type were malformed if the first were true but not one of the others. I think. Can we get a followup to at least add an inline comment to document this, and maybe investigate whether this line is something that should be refactored?

  2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityType.php
    @@ -15,44 +15,17 @@
    +   * The default configuration prefix is constructed from the name of the module
    +   * that provides the entity type and the ID of the entity type. If a
    +   * config_prefix annotation is present it will be used in place of the entity
    +   * type ID.
    
    +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityTypeInterface.php
    @@ -0,0 +1,64 @@
    +   * - The config prefix, which is returned by getConfigPrefix() and is
    +   *   composed of:
    +   *   - The provider module name (limited to 50 characters by
    +   *     DRUPAL_EXTENSION_NAME_MAX_LENGTH).
    +   *   - The module-specific namespace identifier, which defaults to the
    +   *     configuration entity type ID. Entity type IDs are limited to 32
    +   *     characters by EntityTypeInterface::ID_MAX_LENGTH.
    ...
    +   * Ensures that all configuration entities are prefixed by the module that
    +   * provides the configuration entity type. This ensures that if a
    +   * configuration entity is contained in a extension's default configuration it
    +   * be created during extension installation. Additionally, it allows
    +   * dependencies to be calculated without the modules that provide
    +   * configuration entity types being installed.
    

    So the first and third hunks of documentation are added by this patch, and the first is what we have in HEAD. That means we're explaining what a config prefix is composed of in why in different ways in three different places. We should consolidate that and clarify it.

CommentFileSizeAuthor
#7 2482215.2.patch3.23 KBalexpott
#2 2482215.2.patch3.63 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott’s picture

Issue summary: View changes

So re point 1. It is impossible for getConfigEntityPrefix() to return false with the current code. I guess what we need to do here is remove the FALSE possibility from the @return documentation. Also the if the name is not a configuration entity then ConfigManager::getEntityTypeIdByName() returns NULL so it is behaving exactly as documented.

alexpott’s picture

Status: Active » Needs review
FileSize
3.63 KB

Tried to consolidate documentation onto the interface and remove sentences which are no longer true. For example, config entity prefixes now are just part of what causes optional configuration to be installed - but the bigger picture there is that this is all about dependencies - if an optional configurations dependencies are met then it will be installed.

Status: Needs review » Needs work

The last submitted patch, 2: 2482215.2.patch, failed testing.

Status: Needs work » Needs review

alexpott queued 2: 2482215.2.patch for re-testing.

willzyx’s picture

+++ b/.htaccess
@@ -114,7 +114,7 @@ AddEncoding gzip svgz
-  # RewriteBase /
+  RewriteBase /

I think this is the problem

Status: Needs review » Needs work

The last submitted patch, 2: 2482215.2.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
3.23 KB

Lol - that's my environment getting in the way.

jhodgdon’s picture

Status: Needs review » Fixed

This looks good to me. Since alexpott created the patch, I went ahead and committed it (he can't).

  • jhodgdon committed ff62d88 on 8.0.x
    Issue #2482215 by alexpott: Improve config_prefix documentation
    

Status: Fixed » Closed (fixed)

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