-
Notifications
You must be signed in to change notification settings - Fork 21
ISAICP-6532: Adhere to QA coding standard rules #2487
Conversation
…way which is compatible across multiple versions of Drupal.
package: Joinup | ||
core_version_requirement: ^8.9 || ^9 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drupal 9.0 is EOL this month. I think it's safe to use ^8.9 || ^9.1
.
package: Joinup | ||
core_version_requirement: ^8.9 || ^9 | ||
php: 7.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should add this. We should rely on Drupal core PHP minimum version. An exception could be the case when we need in a module version higher than the core min version. E.g. Drupal 8.9 works with PHP 7.1 but the module needs 7.3 in order to work. I guess we don't have such case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a QA coding standard rule that this PHP version has to be defined in the info file, so adding it is in scope of this PR. I agree that it is a bit stupid that we define the default value, but this is maybe something we should propose for the QA coding standards? For me personally it doesn't matter if this line is there or not.
We need minimum PHP 7.1 because we use nullable types and return types. We will for sure adopt 7.4 functionality as soon as we can :D
/** @var \Drupal\file\FileInterface $file */ | ||
$file = File::create(['uri' => $file_path]); | ||
|
||
// The uuid is the one assigned to the user photo field so there is no need |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/uuid/UUID. See https://www.drupal.org/drupalorg/style-guide/content#abbreviations
No description provided.