Follow-up to #2416409: Delete dependent config entities that don't implement onDependencyRemoval() when a config entity is deleted

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityInterface.php
@@ -176,7 +172,11 @@ public function calculateDependencies();
    * @return bool
    *   TRUE if the entity has changed, FALSE if not.
    *
+   * @return bool
+   *   TRUE if the entity has been changed as a result, FALSE if not.
+   *

It's good to add comments, but is the return value double documented?

CommentFileSizeAuthor
#7 interdiff.txt732 bytessnehi
#7 dobule-documentation-2657734-7.patch812 bytessnehi
#6 interdiff-2657734-6-2.txt636 bytesmiteshmap
#6 dobule-documentation-2657734-6.patch674 bytesmiteshmap
#2 2657734-double-documentation-2.patch718 bytesfloydm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lokapujya created an issue. See original summary.

floydm’s picture

floydm CreditAttribution: floydm at Affinity Bridge commented
FileSize
2657734-double-documentation-2.patch718 bytes

Patch attached.

lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya commented

Seems like the one you removed is the newer comment. Only thing is that I wonder if it's better to keep the newer one?

floydm’s picture

floydm CreditAttribution: floydm at Affinity Bridge commented

Beats me. The newer one made less sense to me. "as a result" of what?

lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya commented

So, I commented in the issue that added this to try to recruit some advice. But I think its as a result of changing affected entities in this method. I have to think about the exact wording. So, "as a result of this method", but the newer existing one is nice since it fits on one line.

miteshmap’s picture

miteshmap
Primary language Gujarati
CreditAttribution: miteshmap as a volunteer commented
Status: Active » Needs review
FileSize
dobule-documentation-2657734-6.patch674 bytes
interdiff-2657734-6-2.txt636 bytes

Removed the old one and kept the new one. :)

snehi’s picture

snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commented
FileSize
dobule-documentation-2657734-7.patch812 bytes
interdiff.txt732 bytes

what about this ?

lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya commented
Status: Needs review » Reviewed & tested by the community

Thanks for all the patches. We had an initial patch and someone working on. The exact wording was under discussion. At that point, instead of posting a new patch, you should review the patch and mark it RTBC or suggest a new wording in a comment (and let the person working on it fix it.)

Anyway, I am RTBCing #6 since that it was I was looking for originally.

The last submitted patch, 2: 2657734-double-documentation-2.patch, failed testing.

alexpott’s picture

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

Committed the patch from #6 as the wording change is unnecessary. @snehi I think @lokapujya has a point - I'm not sure what the point of #7 was.

Committed 408cd20 and pushed to 8.0.x and 8.1.x. Thanks!

  • alexpott committed bac3f88 on 8.1.x 
    Issue #2657734 by miteshmap, floydm: calculateDependencies() return...

  • alexpott committed 408cd20 on 
    Issue #2657734 by miteshmap, floydm: calculateDependencies() return...

Status: Fixed » Closed (fixed)

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