Problem/Motivation

Currently we don't apply coding standards checks to files in core/scripts. We have a few instances where PHP code is hiding in files with a .sh extension. This has actually resulted in a bug because the new array syntax has been applied to \Drupal\Core\Locale\CountryManager::getStandardList() but the large array in this file is generated by core/scripts/update-countries.sh

Proposed resolution

Add files to core/phpcs.xml.dist so they are scanned.

Remaining tasks

None

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#2 2935215-2.patch16.23 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Status: Active » Needs review
FileSize
16.23 KB
dawehner’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/phpcs.xml.dist
@@ -2,6 +2,11 @@
+  <file>scripts/update-countries.sh</file>

I'd no idea that this is file is a thing.

I'm wondering whether we should create a followup to have some sort of test for this script.

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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: 2935215-2.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Reviewed & tested by the community

Random test fail due to jenkins database errors.

larowlan’s picture

+++ b/core/phpcs.xml.dist
@@ -2,6 +2,11 @@
+  <file>scripts/drupal.sh</file>
+  <file>scripts/password-hash.sh</file>
+  <file>scripts/rebuild_token_calculator.sh</file>
+  <file>scripts/run-tests.sh</file>
+  <file>scripts/update-countries.sh</file>

Is there a way we can make sure that we don't miss any new files added here? Or can we say that its unlikely we'll use a .sh extension for new php files added to /scripts?

alexpott’s picture

@larowlan I don't think there is a way. You can't use wildcards in the file tag - if you do you get ERROR: The file "scripts/*.sh" does not exist.. I think you're right to think that we're unlikely to add anymore PHP hiding in .sh files because we depend on symfony/console and are using it in things like core/scripts/dump-database-d8-mysql.php and core/scripts/generate-proxy-class.php.

  • larowlan committed 7a72d35 on 8.6.x
    Issue #2935215 by alexpott: Apply coding standards to Drupal 8 PHP code...

  • larowlan committed f25a62f on 8.5.x
    Issue #2935215 by alexpott: Apply coding standards to Drupal 8 PHP code...
larowlan’s picture

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

Committed as 7a72d35 and pushed to 8.6.x

Cherry-picked as f25a62f and pushed to 8.5.x

larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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