API page: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Render%21...

> Merges the values of another bubbleable metadata object with this one.

That makes it sound that the other object is merged into the one on which you make the call. In other words, that you do this:

$my_bubbleable_metadata->merge($other_bubbleable_metadata);
// $my_bubbleable_metadata is now updated! 

That is not the case though. This is what you have to do:

        $my_bubbleable_metadata = $my_bubbleable_metadata->merge($other_bubbleable_metadata);

Can anyone think of how to change the wording of the summary to make this clearer?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim created an issue. See original summary.

christinlepson’s picture

Patch to change "Merges the values of another bubbleable metadata object with this one." to "Returns a new bubbleable metadata object containing the merged data of a given bubbleable metadata object and the current bubbleable metadata object."

christinlepson’s picture

Status: Active » Needs review
borisson_’s picture

Status: Needs review » Needs work

This change is not allowed. The first line of a documentation docblock should only be one 80 cols wide line.

We can make this clearer though, but only if we follow the coding standards for writing documenation.

christinlepson’s picture

Ok a new can be added, @borisson_ did you have any suggestions regarding the wording?

branimirpetkov’s picture

Status: Needs work » Needs review
FileSize
705 bytes

Changed the description.

borisson_’s picture

Status: Needs review » Needs work

I don't have any suggestions no. Writing documentation is really hard and I'm afraid I don't have the required skills to help.

branimirpetkov’s picture

Status: Needs work » Needs review
joachim’s picture

Status: Needs review » Needs work

> Creates a bubbleable metadata object from the merge of this one with another.

is *nearly* short enough.

> Creates a new object from the merge of this one with another.

is there, but we sacrifice saying what kind of object is involved. Is the context sufficient for us to guess? There's the @return docs anyway that will say the class.

christinlepson’s picture

@joachim I believe the context would be sufficient enough given the @return docs.

We could also do "Creates a new bubbleable metadata object by merging this one with another."

joachim’s picture

@clepson I think your suggestion is best -- let's go with that.

borisson_’s picture

Yes I agree, that sounds like a good solution.

christinlepson’s picture

FileSize
667 bytes

Updated docs from

> Merges the values of another bubbleable metadata object with this one.

to

> Creates a new bubbleable metadata object by merging this one with another.

As suggested in #10.

christinlepson’s picture

Status: Needs work » Reviewed & tested by the community
borisson_’s picture

@clepson please don't rtbc your own patches, setting them to needs review so that someone else can mark them as rtbc is the way this usually works.
This way, there are at least 2 people that agree on approach and implementation before it comes to a core committer.

I'm going to leave this on RTBC because I agree with the change though :)

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed 708161c and pushed to 8.8.x. Thanks!

  • catch committed 6595f25 on 8.8.x
    Issue #3060225 by clepson, branimirpetkov: docs summary for...

Status: Fixed » Closed (fixed)

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