Hi.

Problem/Motivation

While migrating a D8.8 site to D9, I encountered the following error while installing the site:
Missing required key (base_theme) in profiles/[...]/mytheme.theme/mytheme.theme

Took me a bit of time to realize the missing key was not base_theme as stated in the error message but base theme (without "_").

Steps to reproduce

  • Create a simple D9 project
  • Remove "base theme" keys from themes' .info.yml files.
  • Run the install process via drush site-install.
  • Expected result: The site install fails with an error message stating the key "base theme" is missing.
  • Actual result: The site install fails with an error message stating the key "base_theme" is missing.

Proposed resolution

Update the error message so it reflects the actual missing key.

CommentFileSizeAuthor
#6 3146567-6.patch1.82 KBalexpott
#6 2-6-interdiff.txt1.82 KBalexpott
#2 wrong-missing-key-name-base-theme-3146567-2.patch1.73 KBpacproduct
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pacproduct created an issue. See original summary.

pacproduct’s picture

pacproduct CreditAttribution: pacproduct commented
Status: Active » Needs review
FileSize
wrong-missing-key-name-base-theme-3146567-2.patch1.73 KB

Here is a pach, as a fix suggestion.

mjpa’s picture

mjpa CreditAttribution: mjpa commented

Came across this just now. Added base_theme as it suggested and still error'd. Patch looks good to me.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Related issues: +#3118957: Evaluate replacing all usage of sprintf by dot concatenation or variable inside string
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Title: Wrong key name in "Missing required key (base_theme)" » Wrong key name in "Missing required key (base_theme)" exception message
Status: Needs review » Reviewed & tested by the community

Good find, looks ready to go to me. Updating the issue title to make it clearer this is just a string issue.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Needs review
FileSize
2-6-interdiff.txt1.82 KB
3146567-6.patch1.82 KB

This is my fault. Sorry. I broke this in #3098250: Remove all @deprecated code in \Drupal\Core\Extension. Looking back at this I think I removed a bit too much of the message. I think we should add See https://www.drupal.org/node/3066038 to the message as people are going to encounter this whilst upgrading so a link to the relevant CR feels pertinent.

pacproduct’s picture

pacproduct CreditAttribution: pacproduct commented

Hey @alexpott :)

Good idea, adding a link to www.drupal.org/node/3066038 would really be helpful.

Considering what's in https://www.drupal.org/project/drupal/issues/3118957 should we not avoid using sprintf?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

The performance of a single sprintf if generating an exception is not really going to matter. If you hit this you have to fix it and then you'll never see it. For me, using sprintf() here makes the code easier to read and check that the link is correct. Which seems better to prioritise in this case.

pacproduct’s picture

pacproduct CreditAttribution: pacproduct commented
Status: Needs review » Reviewed & tested by the community

Fair enough!
Looks good to me then.

xmacinfo’s picture

xmacinfo
he/him
Primary language French
Location Canada
CreditAttribution: xmacinfo at xMac info commented

I have never found that sprintf is easier to read in any piece of PHP I read.

But you are right, displaying an exception using sprintf() or dot concatenation does not matter performance-wize.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

In my background sprintf() is a place where often people displace strings and placeholders

  • catch committed 79835b0 on 9.1.x 
    Issue #3146567 by alexpott, pacproduct: Wrong key name in "Missing...

  • catch committed e619fad on 9.0.x 
    Issue #3146567 by alexpott, pacproduct: Wrong key name in "Missing...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

We don't have a standard on sprintf in exception messages or not (also not sure we should since sometimes 'it depends').

Committed/pushed to 9.1.x and cherry-picked to 9.0.x, thanks!

Status: Fixed » Closed (fixed)

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