Problem/Motivation

Config (entities) can depend on modules, themes as well as entities. For some time, it could not depend on content entities, only config entities, so they $type 'entity' was used. Since content entities was also supported, 'entity' was changed to 'content' and 'config' to distinguish the cases. But still, the method documentation of DependencyTrait::addDependency() mentions only 'entity'.

Proposed resolution

Change 'entity' to 'config' and 'content' there.

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#15 dependencytrait-documentation-2485403-15.patch840 bytesmmatsoo
#12 dependencytrait-documentation-2485403-12.patch883 bytesmmatsoo
#8 dependencytrait-documentation-2485403-8.patch887 bytesmmatsoo
#6 dependencytrait-documentation-2485403-5.patch838 bytesmmatsoo
#4 dependencytrait-documentation-2485403-4.patch862 bytesmmatsoo
#1 dependencytrait-documentation-2485403-1.patch821 bytesgaurav_varshney
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

gaurav_varshney’s picture

gaurav_varshney CreditAttribution: gaurav_varshney commented
Status: Active » Needs review
FileSize
dependencytrait-documentation-2485403-1.patch821 bytes
eojthebrave’s picture

eojthebrave
he/him
Primary language English
Location Minneapolis, MN
CreditAttribution: eojthebrave commented
Status: Needs review » Needs work

Thanks for the patch.

+++ b/core/lib/Drupal/Core/Entity/DependencyTrait.php
@@ -23,10 +23,10 @@
+   *   The type of dependency being added: 'module', 'theme', or 'config' and 'content'.

This line should wrap at 80 characters per the code standards.

I also find the wording a little off, maybe just ... "theme, config, or content" and remove the "and"?

+++ b/core/lib/Drupal/Core/Entity/DependencyTrait.php
@@ -23,10 +23,10 @@
+   *   $type is 'config' and 'content', the full configuration object name.

I think this should be "config or content" not "config and content". And, should this be something like "the full content object" or similar if this is of the type 'content'?

mmatsoo’s picture

mmatsoo CreditAttribution: mmatsoo as a volunteer commented
Assigned: Unassigned » mmatsoo
mmatsoo’s picture

mmatsoo CreditAttribution: mmatsoo as a volunteer commented
Assigned: mmatsoo » Unassigned
Status: Needs work » Needs review
FileSize
dependencytrait-documentation-2485403-4.patch862 bytes

Changed and shortened text.

Status: Needs review » Needs work

The last submitted patch, 4: dependencytrait-documentation-2485403-4.patch, failed testing.

mmatsoo’s picture

mmatsoo CreditAttribution: mmatsoo as a volunteer commented
Status: Needs work » Needs review
FileSize
dependencytrait-documentation-2485403-5.patch838 bytes

Trying again. Not sure why last patch failed testing.

Status: Needs review » Needs work

The last submitted patch, 6: dependencytrait-documentation-2485403-5.patch, failed testing.

mmatsoo’s picture

mmatsoo CreditAttribution: mmatsoo as a volunteer commented
Status: Needs work » Needs review
FileSize
dependencytrait-documentation-2485403-8.patch887 bytes

One more time. After this I can't imagine why the patch keeps failing.

iMiksu’s picture

iMiksu
Location Finland
CreditAttribution: iMiksu as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Docs and patch looking good.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/lib/Drupal/Core/Entity/DependencyTrait.php
@@ -23,10 +23,10 @@
+   *   If $type is 'config' or 'content', the full configuration object name.

Should be the result of getConfigDependencyName(). For configuration entities this is the configuration object name - but for content entities this is entity_type:bundle:UUID. Also this line should wrap with the previous line.

mmatsoo’s picture

mmatsoo CreditAttribution: mmatsoo as a volunteer commented
Assigned: Unassigned » mmatsoo
mmatsoo’s picture

mmatsoo CreditAttribution: mmatsoo as a volunteer commented
Assigned: mmatsoo » Unassigned
Status: Needs work » Needs review
FileSize
dependencytrait-documentation-2485403-12.patch883 bytes

Updated text and got rid of unnecessary line feed.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs review » Needs work 
+++ b/core/lib/Drupal/Core/Entity/DependencyTrait.php
@@ -23,10 +23,11 @@
+   *   $type is 'config', the full configuration object name. If $type is ¶
+   *   'content', then the string is formatted entity_type:bundle:UUID.
...
    * @see \Drupal\Core\Entity\EntityInterface::getConfigDependencyName()

I think we should just say that this is the result of EntityInterface::getConfigDependencyName() since what that actually returns is an implementation detail.

mmatsoo’s picture

mmatsoo CreditAttribution: mmatsoo as a volunteer commented
Assigned: Unassigned » mmatsoo
mmatsoo’s picture

mmatsoo CreditAttribution: mmatsoo as a volunteer commented
Assigned: mmatsoo » Unassigned
Status: Needs work » Needs review
FileSize
dependencytrait-documentation-2485403-15.patch840 bytes

Switched text to be closer to original where config and content types are documented in the same sentence, now as the result of EntityInterface::getConfigDependencyName().

eojthebrave’s picture

eojthebrave
he/him
Primary language English
Location Minneapolis, MN
CreditAttribution: eojthebrave commented
Status: Needs review » Reviewed & tested by the community

Looks good to me. Current patch addresses all the concerns so far and the comments are correctly formatted. So I think this is RTBC.

FWIW. I haven't actually applied this locally, but it's just a documentation change, and the test bot says that it is able to apply the patch so I think this is safe.

  • alexpott committed 34a5bd1 on 8.0.x 
    Issue #2485403 by mmatsoo, gaurav_varshney: DependencyTrait...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed 34a5bd1 and pushed to 8.0.x. Thanks!

Status: Fixed » Closed (fixed)

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