This is a follow-up from https://www.drupal.org/node/2572699#comment-12271753.

The line:
for ($id = $original_id; $container->hasAlias($id); $id = (string) $container->getAlias($id));
should be more readable.

CommentFileSizeAuthor
#10 2911498_10.patch988 bytesmile23
#10 interdiff.txt749 bytesmile23
#7 2911498_7.patch1.42 KBmile23

Comments

mfernea created an issue. See original summary.

dawehner’s picture

dawehner
Primary language German
commented

Can you explain why this is confusing for you? For me this is a normal for loop.

  • A start
  • A condition
  • An iterator
mfernea’s picture

mfernea commented

I guess the problem is that the loop doesn't have a body.
As mentioned in the issues summary this is a follow-up on @catch's request as

we need a follow-up ... exposed very unreadable code

dawehner’s picture

dawehner
Primary language German
commented

Ah I see, this makes more sense. Thank you for the clarification.

Just a random idea ..., this could be a while loop inside a helper method ... I think this would make things clearer.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Related issues: +#2907169: Break KernelTestBase dependency on Simpletest, deprecate stub BC traits

We're moving this file around in #2907169: Break KernelTestBase dependency on Simpletest, deprecate stub BC traits

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Title: TestServiceProvider.php - unreadable code » Make TestServiceProvider more readable (cleanup)
Status: Active » Needs review
StatusFileSize
new 2911498_7.patch1.42 KB

A patch. Includes updates to the class docblock.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

This is a lot more readable, great work here!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/tests/Drupal/KernelTests/TestServiceProvider.php
@@ -8,7 +8,11 @@
+ * This provider will trigger a rebuild of the container for TestBase-based

This is not quite right. It calls a test method's containerBuild method when a container is being built. It does not trigger a rebuild. Also this only makes sense in a KernelTestBase as this class could never affect the site under test.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs work » Needs review
Related issues: +#2953098: Split TestServiceProvider into two service providers
StatusFileSize
new interdiff.txt749 bytes
new 2911498_10.patch988 bytes

OK. We'll split the difference, literally.

Reverted the docblock update and filed #2953098: Split TestServiceProvider into two service providers so that we can further separate the side-effects and prepare for simpletest deprecation.

Status: Needs review » Needs work

The last submitted patch, 10: 2911498_10.patch, failed testing. View results

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs work » Needs review

Setting to NR after testbot hiccup.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

Back to rtbc, this is more readable and the part of the patch that @alexpott had issues with is removed.

alexpott credited catch.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Updating issue credit for comments that affected the patch. Also crediting @catch since this tidy up was @catch's idea.

alexpott’s picture

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

Committed c7b8d93 and pushed to 8.6.x. Thanks!

  • alexpott committed c7b8d93 on 8.6.x 
    Issue #2911498 by Mile23, mfernea, alexpott, dawehner, catch: Make...

Status: Fixed » Closed (fixed)

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