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

The docblock should say what the assumptions are, and the code should document this hideous expression:

dirname(dirname(substr(__DIR__, 0, -strlen(__NAMESPACE__))));

CommentFileSizeAuthor
#13 DrupalKernel--guessApplicationRoot-c-updated-2905109-13.patch895 bytesmsankhala
#12 2905109-12.patch797 bytesgawaksh
#9 2905109-9.patch722 bytesgawaksh
#5 2905109-4.patch1.05 KBiainp999
#4 2905109-3.patch1007 bytesiainp999
#2 2905109-2.patch1.02 KBiainp999
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim created an issue. See original summary.

iainp999’s picture

iainp999 CreditAttribution: iainp999 as a volunteer commented
FileSize
2905109-2.patch1.02 KB

Hi @joachim

Are the comments in the attached patch correct? Do they cover this or is there more to say?

Thanks.

iainp999’s picture

iainp999 CreditAttribution: iainp999 as a volunteer commented
Status: Active » Needs review
iainp999’s picture

iainp999 CreditAttribution: iainp999 as a volunteer commented
FileSize
2905109-3.patch1007 bytes

Oops, sorry. Initial docblock wasn't quite right. New patch attached. (Didn't bother with interdiff, since this is small).

iainp999’s picture

iainp999 CreditAttribution: iainp999 as a volunteer commented
FileSize
2905109-4.patch1.05 KB

Or perhaps the attached is better?

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.

borisson_’s picture

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

I agree, I think this is a good idea to add the assumptions in the documentation of this method.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -299,10 +299,21 @@ public function __construct($environment, $class_loader, $allow_dumping = TRUE,
    * Determine the application root directory based on assumptions.
    *
+   * This implementation assumes that the application root is two levels
+   * higher in the directory structure than the PSR-4 base directory (i.e. two
+   * levels up from the path to this class in the filesystem minus it's
+   * namespace).
...
+    // Guess the application root by:
+    //
+    // - removing the namespace directories from the path
+    // - getting the path to the directory two levels up from the path
+    //   determined in the previous step.

I don't think repeating the documentation about the implementation helps. It means that if we can the implementation we have to change documentation. I think what I'd do is change Determine the application root directory based on assumptions. to Determine the application root directory based on this file's location. and then only have the inline comments next to the code as the implementation is probably not that important to the caller.

The inline comments current don't obey the API docs standard - there is no empty line in-between the colon and that first hyphen. See https://www.drupal.org/docs/develop/coding-standards/api-documentation-a...

gawaksh’s picture

gawaksh CreditAttribution: gawaksh at OpenSense Labs for DrupalFit commented
FileSize
2905109-9.patch722 bytes

This patch might fulfil the requirements.

gawaksh’s picture

gawaksh CreditAttribution: gawaksh at OpenSense Labs for DrupalFit commented
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented

The suggestion in #9 doesn't do what @alexpott suggested in #8.

gawaksh’s picture

gawaksh CreditAttribution: gawaksh at OpenSense Labs for DrupalFit commented
Assigned: Unassigned » gawaksh
Status: Needs work » Needs review
FileSize
2905109-12.patch797 bytes

Here's a fresh patch after the changes.

msankhala’s picture

msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commented
FileSize
DrupalKernel--guessApplicationRoot-c-updated-2905109-13.patch895 bytes

Here is a patch as per suggestion in #8 by @alexpott.

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

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

joachim’s picture

joachim CreditAttribution: joachim as a volunteer commented
Status: Needs review » Reviewed & tested by the community

LGTM. Thanks!

  • alexpott committed 03a3058 on 8.7.x 
    Issue #2905109 by iainp999, gawaksh, msankhala, joachim, borisson_,...

  • alexpott committed 837247a on 8.6.x 
    Issue #2905109 by iainp999, gawaksh, msankhala, joachim, borisson_,...
alexpott’s picture

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

Committed 03a3058 and pushed to 8.7.x and 8.6.x. Thanks!

Status: Fixed » Closed (fixed)

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