Problem/Motivation

When placing block on a page, you can set the block machine name. But the configuration of this block depends on the theme you place your block in.

On the same websites, if there are multiple themes, you can't place the same block on another theme with the same machine name because it is already used.

Proposed resolution

A best practice would be to prefix block machine name by the machine name of the theme it is in.

For example: block.block.bartik_help.yml

Remaining tasks

  • Review patch
  • Update core umami and testing modules
CommentFileSizeAuthor
#9 interdiff-3061184-3-9.txt4.51 KBgrimreaper
#9 coder-block_machine_name-3061184-9.patch7.24 KBgrimreaper
#3 coder-block_machine_name-3061184-3.patch6.54 KBgrimreaper
#2 coder-block_machine_name-3061184-2.patch6.18 KBgrimreaper

Comments

Grimreaper created an issue. See original summary.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Assigned: grimreaper » Unassigned
Status: Active » Needs review
StatusFileSize
new coder-block_machine_name-3061184-2.patch6.18 KB

Here is a first patch.

It will fail. I can't find why the new rule is not applied.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Issue summary: View changes
StatusFileSize
new coder-block_machine_name-3061184-3.patch6.54 KB

Fixed the preg_match pattern that prevents the new sniff to work.

Using YAML::parse() following @klausi recommendations.

Thanks for the review.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Status: Needs review » Postponed

Postponed until related issues are green.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Issue tags: +DevDaysTransylvania

Tagging.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

I'm not convinced this rule belongs in coder. I think the core change to add the theme to the suggestions is fine but I don't think we need a coder rule here.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

Is there a place in Coder to have rules not enabled by default and so people can opt-in for these rules with a custom phpcs.xml config file?

Because at the beginning it is only what I wanted, a rule for custom projects. If people don't want to use this rule, no problem.

I know, for example I encountered someone mentionning it in a presentation at DrupalCon Europe 2020, that there is other people that might be interested in this rule.

If we can avoid having everyone get their one rules in separated packages, I think that may be helpful. If not, fine, I will create my own package.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

I just found a problem with previous patch.

When the file is a config translation it raises a warning:

FILE: project/conf/drupal/default/sync/language/fr/block.block.bartik_account_menu.yml
---------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
---------------------------------------------------------------------------------------------------------------------------------------------------------------------
 1 | WARNING | The machine name of the block should be prefixed by the machine name of the theme it is placed in, separated with an underscore. Example: bartik_help
---------------------------------------------------------------------------------------------------------------------------------------------------------------------

It should ignore config translation files.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
StatusFileSize
new coder-block_machine_name-3061184-9.patch7.24 KB
new interdiff-3061184-3-9.txt4.51 KB

Fixing the case of config translation file and updating the code regarding changes in Coder since last patch.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Status: Postponed » Needs review

Changing to needs review to have some feedback from maintainers.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Needs work

Thanks, can you create a pull request against https://github.com/pfrenssen/coder so that we can see the test cases run?

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

Hello,

@klausi thanks for the feedback.

PR created https://github.com/pfrenssen/coder/pull/151

Also have you seen comments 6 and 7?

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs work » Needs review

Thanks - I'm also not fully convinced this rule should be in Coder. I don't think people execute Coder on their config sync directory YAML files?

On the other hand I also don't see much harm in adding this rule - if it sees a block.block.*.yml file then it reports on that. Not much that can go wrong with that.

Anyone else that has opinions and wants this?

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

Hi,

FYI, I have created my own Composer package to provide this rule, so no more need to place it in Coder.

https://github.com/FlorentTorregrosa/phpcs-drupal

I let maintainers choose of the new issue's status. Should it be considered as "Fixed" or as "Closed (won't fix)"?

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Closed (won't fix)

Thanks, then I'll close this as won't fix for the time being. We can always considers this later again if needed.