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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mfernea created an issue. See original summary.

dawehner’s picture

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

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

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’s picture

Title: TestServiceProvider.php - unreadable code » Make TestServiceProvider more readable (cleanup)
Status: Active » Needs review
FileSize
1.42 KB

A patch. Includes updates to the class docblock.

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

This is a lot more readable, great work here!

alexpott’s picture

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

Status: Needs work » Needs review
Related issues: +#2953098: Split TestServiceProvider into two service providers
FileSize
749 bytes
988 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

Status: Needs work » Needs review

Setting to NR after testbot hiccup.

borisson_’s picture

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

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

alexpott’s picture

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.