CommentFileSizeAuthor
#20 docblock_and_code_style-2210565-20.patch8.79 KBsushyl
#17 D8-2210565-17-bootstrap-inc-combined.patch9.47 KBsushyl
#15 D8-2210565-15-bootstrap-inc-docblock.patch25.49 KBdonquixote
#15 D8-2210565-15-bootstrap-inc-codestyle.patch5.14 KBdonquixote
#15 D8-2210565-15-bootstrap-inc-combined.patch30.11 KBdonquixote
#7 D8-2210565-7-bootstrap-inc-codestyle.patch5.45 KBdonquixote
#7 D8-2210565-7-bootstrap-inc-docblock.patch25.49 KBdonquixote
#7 D8-2210565-7-bootstrap-inc-combined.patch30.42 KBdonquixote
#1 D8-2210565-1-bootstrap-inc-code-style.patch16.99 KBdonquixote
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

donquixote’s picture

donquixote CreditAttribution: donquixote commented
Status: Active » Needs review
FileSize
D8-2210565-1-bootstrap-inc-code-style.patch16.99 KB
donquixote’s picture

donquixote CreditAttribution: donquixote commented

Btw, another issue:
PhpStorm (any mabye other IDEs, who knows) has problems if a class alias has the same name as a function.
(and this is not case sensitive, for obvious reasons)
E.g.

use Drupal\Component\Utility\Settings;
use Drupal\Core\Language\Language;
[..]
function settings() {
[..]
function language() {
[..]

One way out could be to rename these two aliases.

donquixote’s picture

donquixote CreditAttribution: donquixote commented

This commit can fix the issue mentioned in #2:
https://github.com/donquixote/drupal/commit/1e77fe7e3eddadefab1e480340db...

Cameron Tod’s picture

Cameron Tod CreditAttribution: Cameron Tod commented
Status: Needs review » Closed (duplicate) 
  1. +++ b/core/includes/bootstrap.inc
@@ -1,28 +1,19 @@
 use Drupal\Component\Utility\Crypt;
-use Drupal\Component\Utility\NestedArray;
 use Drupal\Component\Utility\Settings;
 use Drupal\Component\Utility\String;
 use Drupal\Component\Utility\Timer;
 use Drupal\Component\Utility\Unicode;
-use Drupal\Component\Utility\Url;
 use Drupal\Core\DrupalKernel;
-use Drupal\Core\Database\Database;
-use Drupal\Core\DependencyInjection\ContainerBuilder;
 use Drupal\Core\Extension\ExtensionDiscovery;
 use Drupal\Core\Utility\Title;
 use Drupal\Core\Utility\Error;
 use Symfony\Component\ClassLoader\ApcClassLoader;
-use Symfony\Component\DependencyInjection\ContainerInterface;
-use Symfony\Component\DependencyInjection\Container;
-use Symfony\Component\DependencyInjection\Reference;
 use Symfony\Component\DependencyInjection\Exception\RuntimeException as DependencyInjectionRuntimeException;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\Response;
 use Drupal\Core\Language\Language;
-use Drupal\Core\Lock\DatabaseLockBackend;
-use Drupal\Core\Lock\LockBackendInterface;
 use Drupal\Core\Session\UserSession;

    Great to see these unused classes not taking up cognitive space. 

  2. +++ b/core/includes/bootstrap.inc
@@ -473,7 +465,9 @@ function drupal_environment_initialize() {
+ * @param string
+ *

    Shouldn't this parameter be documented? 

  3. -  return $allow_caching_static && ($_SERVER['REQUEST_METHOD'] == 'GET' || $_SERVER['REQUEST_METHOD'] == 'HEAD')
+  return $allow_caching_static
+    && ($_SERVER['REQUEST_METHOD'] == 'GET' || $_SERVER['REQUEST_METHOD'] == 'HEAD')
     && !drupal_is_cli();

    +++ b/core/includes/bootstrap.inc
@@ -2524,7 +2542,9 @@ function drupal_check_memory_limit($required, $memory_limit = NULL) {
+  return !$memory_limit
+    || $memory_limit == -1
+    || parse_size($memory_limit) >= parse_size($required);

    This is much more readable, but I'm not sure if matches Drupal's coding standard.

Grepping through the D8 codebase, there's a lot of code that needs updating the new standards here: #711918: Documentation standard for @param and @return data types

More broadly though, there seems to be an issue for this (and the rest of core) already: #1518116: [meta] Make Core pass Coder Review

donquixote’s picture

donquixote CreditAttribution: donquixote commented

#711918: Documentation standard for @param and @return data types
discusses the introduction of the standards but not their implementation

#1518116: [meta] Make Core pass Coder Review
is a meta issue, no patches going to happen there.

#1533094: Make includes directory, files starting with A-C pass Coder Review
this seems like exactly the right place!

donquixote’s picture

donquixote CreditAttribution: donquixote commented
Status: Closed (duplicate) » Active

The other issues are a dead end.
#1326456-29: Add missing @param in includes A-C, plus other corrections to some docblocks

I talked with jhodgdon on IRC.
The conclusion:
This family of issues should remain closed, because it is an approach that failed. This does exactly confirm the concerns I expressed above.
Instead, issues like #2210565: Docblock and code style improvements in core/includes/bootstrap.inc are exactly the way to go. So I am going to re-open that!

I worked some more on the patch and I am going to post one that focuses on the docblock.

donquixote’s picture

donquixote CreditAttribution: donquixote commented
FileSize
D8-2210565-7-bootstrap-inc-combined.patch30.42 KB
D8-2210565-7-bootstrap-inc-docblock.patch25.49 KB
D8-2210565-7-bootstrap-inc-codestyle.patch5.45 KB

Combined branch with docblock AND code style improvements merged together:
https://github.com/donquixote/drupal/compare/8.x-bootstrap-inc

Only code style:
https://github.com/donquixote/drupal/compare/8.x-codestyle-bootstrap-inc
This part contains some controversial commits that I would like to get further opinions on.

Only docblock:
https://github.com/donquixote/drupal/compare/8.x-docblock-bootstrap-inc

@everyone, reviewers:
Please let me know if you want to focus on docblock only or the combined patch.

donquixote’s picture

donquixote CreditAttribution: donquixote commented
Status: Active » Needs review

"needs review"

The last submitted patch, 7: D8-2210565-7-bootstrap-inc-combined.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 7: D8-2210565-7-bootstrap-inc-codestyle.patch, failed testing.

donquixote’s picture

donquixote CreditAttribution: donquixote commented
Status: Needs work » Needs review

7: D8-2210565-7-bootstrap-inc-codestyle.patch queued for re-testing.

donquixote’s picture

donquixote CreditAttribution: donquixote commented
Status: Needs review » Needs work

https://github.com/donquixote/drupal/commit/1fc4f5453351b43fd9a3227dc963...

      if (is_object($cache)) {
 +      if (!$cache instanceof stdClass) {
 +        $cache_class = get_class($cache);
 +        throw new \Exception("\$cache must be an instance of stdClass, not $cache_class.");
 +      }

drupal_page_get_cache() returns an "object" with no type specified, whereas drupal_serve_page_from_cache($cache, ..) requires a stdClass object as the first parameter. Just assuming that object is a stdClass seems wrong. But the proposed solution seems overkill.

The last submitted patch, 7: D8-2210565-7-bootstrap-inc-codestyle.patch, failed testing.

donquixote’s picture

donquixote CreditAttribution: donquixote commented

The "Remove local variables" on drupal_settings_initialize() was a bad idea.
https://github.com/donquixote/drupal/commit/dc67d8827d194c5b860e6147a72b...
The "unused" variables are used in settings.php.

I am not sure for watchdog(). I don't see any require/include or compact() or anything, so I assume that $base_root is really not used anywhere in the function.

donquixote’s picture

donquixote CreditAttribution: donquixote commented
Status: Needs work » Needs review
FileSize
D8-2210565-15-bootstrap-inc-combined.patch30.11 KB
D8-2210565-15-bootstrap-inc-codestyle.patch5.14 KB
D8-2210565-15-bootstrap-inc-docblock.patch25.49 KB

https://github.com/donquixote/drupal/compare/8.x-bootstrap-inc

EDIT: As already mentioned in #7:

Please let me know if I should focus on docblock only or the combined patch.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 commented
Status: Needs review » Needs work
Issue tags: +Needs reroll
sushyl’s picture

sushyl
Location Pune
CreditAttribution: sushyl at Axelerant commented
Issue summary: View changes
Status: Needs work » Needs review
FileSize
D8-2210565-17-bootstrap-inc-combined.patch9.47 KB

Rerolled Combined from #15 patch. Uploading patch for testing.

googletorp’s picture

googletorp CreditAttribution: googletorp at Reveal IT commented
Issue tags: -Needs reroll

Status: Needs review » Needs work

The last submitted patch, 17: D8-2210565-17-bootstrap-inc-combined.patch, failed testing.

sushyl’s picture

sushyl
Location Pune
CreditAttribution: sushyl at Axelerant commented
Status: Needs work » Needs review
FileSize
docblock_and_code_style-2210565-20.patch8.79 KB

8.0.x/8.1.x

Rerolled patch

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

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

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs review » Closed (duplicate)

Thanks for the patch. However, we have a number of issues dealing with coding standards fixes and the community has decided that the best way to approach this is by fixing a rule at a time, rather than a file at a time. See #2571965: [meta] Fix PHP coding standards in core for the meta issue where this effort is being organized,

Closing this as a duplicate of #3207949: Fix Drupal.Commenting.FunctionComment.MissingParamType and adding credit over there.