Problem/Motivation

The return docs for \Drupal\layout_builder\Section::getComponents() say:

   * @return \Drupal\layout_builder\SectionComponent[]
   *   The components.

This should say the array is keyed by the component UUID.

CommentFileSizeAuthor
#18 interdiff_15-18.txt1.99 KBravi.shankar
#18 3285593-18.patch563 bytesravi.shankar
#15 interdiff-11-15.txt1.75 KBrassoni
#15 3285593-15.patch2.3 KBrassoni
#11 3285593-11.patch557 bytesmrinalini9
#9 interdiff_7-9.txt880 bytesashutosh ahirwal
#9 3285593-9.patch561 bytesashutosh ahirwal
#7 interdiff_6-7.txt456 bytesimmaculatexavier
#7 3285593-7.patch512 bytesimmaculatexavier
#6 interdiff_4-6.txt513 bytesimmaculatexavier
#6 3285593-6.patch557 bytesimmaculatexavier
#4 3285593-4.patch546 bytesimmaculatexavier

Issue fork drupal-3285593

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

joachim created an issue. See original summary.

immaculatexavier made their first commit to this issue’s fork.

immaculatexavier opened merge request !2392

immaculatexavier’s picture

immaculatexavier commented
Status: Active » Needs review
StatusFileSize
new 3285593-4.patch546 bytes

Attached the patch

joachim’s picture

joachim commented
Status: Needs review » Needs work

Thanks for the patch!

+++ b/core/modules/layout_builder/src/Section.php
@@ -167,7 +167,7 @@ public function getDefaultRegion() {
-   *   The components.
+   *   The array is keyed by the component UUID.

This is removing the description, which isn't the right thing to do.

It should say something like 'An array of components, keyed by the component UUID.'

immaculatexavier’s picture

immaculatexavier commented
Status: Needs work » Needs review
StatusFileSize
new 3285593-6.patch557 bytes
new interdiff_4-6.txt513 bytes

Addressed #5

immaculatexavier’s picture

immaculatexavier commented
StatusFileSize
new 3285593-7.patch512 bytes
new interdiff_6-7.txt456 bytes

Now the code will be as below:

+++ b/core/modules/layout_builder/src/Section.php
@@ -168,6 +168,7 @@ public function getDefaultRegion() {
    *
    * @return \Drupal\layout_builder\SectionComponent[]
    *   The components.
+   *   An array of components, keyed by the component UUID.
joachim’s picture

joachim commented
Status: Needs review » Needs work 
+++ b/core/modules/layout_builder/src/Section.php
@@ -168,6 +168,7 @@ public function getDefaultRegion() {
    *   The components.
+   *   An array of components, keyed by the component UUID.

Now you're leaving the original line in, even though the new line is repeating its meaning.

It should still say it's components, but it shouldn't say it twice.

The complete description can just be:

> An array of components, keyed by the component UUID.

That tells us that it's an array, what is in the array, and what the keys are.

ashutosh ahirwal’s picture

ashutosh ahirwal commented
Status: Needs work » Needs review
StatusFileSize
new 3285593-9.patch561 bytes
new interdiff_7-9.txt880 bytes

Attach this patch.

joachim’s picture

joachim commented
Status: Needs review » Needs work

Thanks!

The wording is fine now, but the indentation has been changed.

mrinalini9’s picture

mrinalini9 commented
Status: Needs work » Needs review
StatusFileSize
new 3285593-11.patch557 bytes

Hi,

I have uploaded the patch by fixing above mentioned indentation issue in #10, please review it.

Thanks & Regards,
Mrinalini

Status: Needs review » Needs work

The last submitted patch, 11: 3285593-11.patch, failed testing. View results

joachim’s picture

joachim commented
Status: Needs work » Reviewed & tested by the community

Test failure is unrelated, and this is a docs-only patch.

This looks perfect. Thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: 3285593-11.patch, failed testing. View results

rassoni’s picture

rassoni commented
Status: Needs work » Needs review
StatusFileSize
new 3285593-15.patch2.3 KB
new interdiff-11-15.txt1.75 KB

Due to test failure unrelated to this issue converted to need to work.
I have added a throw statement and added interdiff.

joachim’s picture

joachim commented

Adding the @throws is outside of this issue's scope, sorry. Also, IIRC, a @throws tag needs a line after it explaining when the exception is thrown.

joachim’s picture

joachim commented
Status: Needs review » Needs work
ravi.shankar’s picture

ravi.shankar commented
Status: Needs work » Needs review
StatusFileSize
new 3285593-18.patch563 bytes
new interdiff_15-18.txt1.99 KB

Removed @throws from patch #15 as per comment #16, please review.

joachim’s picture

joachim commented
Status: Needs review » Reviewed & tested by the community

That's great, thanks!

(And feel free to file a new issue for the @throws!)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 10.0.x-dev » 9.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed f667b7d0ef to 10.1.x and ffb69c6b96 to 10.0.x and 226bec73b9 to 9.5.x and 05d1c39075 to 9.4.x. Thanks!

  • alexpott committed 616d5e0 on 10.1.x 
    Issue #3285593 by immaculatexavier, Ashutosh Ahirwal, Rashmisoni, ravi....

  • alexpott committed ffb69c6 on 10.0.x 
    Issue #3285593 by immaculatexavier, Ashutosh Ahirwal, Rashmisoni, ravi....

  • alexpott committed 226bec7 on 9.5.x 
    Issue #3285593 by immaculatexavier, Ashutosh Ahirwal, Rashmisoni, ravi....

  • alexpott committed 05d1c39 on 9.4.x 
    Issue #3285593 by immaculatexavier, Ashutosh Ahirwal, Rashmisoni, ravi....

Status: Fixed » Closed (fixed)

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