Issue #3341682: New config schema data type: `required_label`
Merge request reports
Activity
added 1 commit
- 6c4394c6 - Make `Block` pass tests by adding the missing label.
added 1 commit
- 8fc9f0b7 - Add explicit test coverage — both for config entities that store the label in...
added 2 commits
added 1 commit
- 22e74b89 - Some config entity types truly do not have labels.
added 1 commit
- 5630efac - 3 config entity types have been using `label` incorrectly, switch them to `text`.
added 1 commit
- eb066c37 - `system.site`: `slogan` should not use `type: label`, `name` is indeed...
added 1 commit
- 1d45c2bd - When the `label` entity key is the same as the `id` entity key, labellessness is impossible.
added 1 commit
- b6999cb8 - `settings.prefix` & `settings.suffix` should also not have been using `type: label`.
- Resolved by Wim Leers
- Resolved by Wim Leers
168 171 172 /** 173 * Tests validation of config entity's label. 174 * 175 * @see \Drupal\Core\Entity\EntityInterface::label() 176 * @see \Drupal\Core\Entity\EntityBase::label() 177 */ 178 public function testLabelValidation(): void { 179 // Some entity types are labelless: 180 // - \Drupal\Core\Entity\Entity\EntityFormDisplay 181 // - \Drupal\Core\Entity\Entity\EntityViewDisplay 182 // - \Drupal\language\Entity\ContentLanguageSettings 183 // - \Drupal\rest\Entity\RestResourceConfig 184 if ($this->entity->label() === NULL) { 185 $this->markTestSkipped(); 186 } Darn, good catch! Although … that would have meant that every other test method would've triggered a validation error for the missing label!
So it was only a theoretical problem. Still, this code on its own is definitely confusing.
So made it more explicit in f2e06614.
changed this line in version 12 of the diff
- Resolved by Wim Leers
- Resolved by Wim Leers
186 } 187 // @todo Remove this in https://www.drupal.org/i/3231354. 188 if ($this->entity->getEntityType()->id() === 'editor') { 189 $this->markTestSkipped(); 190 } 191 // @todo Remove this in https://www.drupal.org/i/2939931. 192 if ($this->entity instanceof LayoutBuilderEntityViewDisplay) { 193 $this->markTestSkipped(); 194 } 195 if ($this->entity->getEntityType()->getKey('label') === $this->entity->getEntityType()->getKey('id')) { 196 $this->markTestSkipped('This entity type uses the ID as the label; a labelless entity is hence impossible.'); 197 } 198 $label_property = $this->entity->getEntityType()->getKey('label'); 199 if ($label_property === FALSE && !method_exists($this, 'removeLabel')) { 200 throw new \LogicException(sprintf('Please implement a removeLabel() method to allow testing a labelless %s.', (string) $this->entity->getEntityType()->getSingularLabel())); 201 } - Resolved by Wim Leers
- Resolved by Wim Leers
- Resolved by Wim Leers
added 4 commits
- 865ae028 - Per @borisson_ review: Avoid the need for a `cspell:ignore`.
- f2e06614 - Per @phenaproxima review: Make the test exemption logic for entity types...
- 597e8bf7 - Per @phenaproxima review: do not hardcode edge cases in base test, move them...
- 61bfd2f3 - Per @phenaproxima & @borisson_ review: refactor the `removeLabel()` chaos into...
Toggle commit listadded 1 commit
- d394d6f0 - Introduce `type: translatable_string` — see CR at https://www.drupal.org/node/3349638.
- Resolved by Wim Leers
added 219 commits
-
d394d6f0...53f13043 - 218 commits from branch
project:10.1.x - 7f295af1 - Merge remote-tracking branch 'origin/10.1.x' into 3341682-label-type-validation
-
d394d6f0...53f13043 - 218 commits from branch
added 1 commit
- 7b3fc9cc - Convert all uses of `{ type: string, translatable: true }` to `{ type: translatable_string }`.
added 67 commits
-
7b3fc9cc...da2eaec5 - 66 commits from branch
project:10.1.x - 1e634756 - Merge remote-tracking branch 'origin/10.1.x' into 3341682-label-type-validation
-
7b3fc9cc...da2eaec5 - 66 commits from branch
added 1 commit
- Resolved by Wim Leers
- Resolved by Wim Leers
added 3 commits
- bcdf42a8 - Add clarifications requested by @borisson_.
- b5c85b12 - Fix test failure.
- d43112fd - The changed approach to `required_label` enables removing some changes: tighten scope.
added 338 commits
-
d43112fd...e646c8e5 - 337 commits from branch
project:11.x - 36425556 - Merge remote-tracking branch 'origin/11.x' into 3341682-label-type-validation
-
d43112fd...e646c8e5 - 337 commits from branch
added 1 commit
-
d70d7845 - Drupal 10.2 no longer allows the word "please"
♂️
-
d70d7845 - Drupal 10.2 no longer allows the word "please"
added 7 commits
-
db1d7b89...85700fc5 - 6 commits from branch
project:11.x - 305eb8a0 - Merge remote-tracking branch 'origin/11.x' into 3341682-label-type-validation
-
db1d7b89...85700fc5 - 6 commits from branch
added 5 commits
- cdbe4d66 - `NodeType` config entities REQUIRE a `name` property to be set.
- e03115f7 - `CommentType` config entities REQUIRE a `label` property to be set.
- 65358755 - `ImageStyle` config entities REQUIRE a `label` property to be set.
- 63924604 - `ConfigurableLanguage` config entities REQUIRE a `label` property to be set.
- 08139f6b - Assortment of other "missing config entity label" example fixes.
Toggle commit listadded 1 commit
- d2d62214 - Add section labels in demo_umami page layouts
added 10 commits
- dac602f3 - Fix FieldTypeTest
- 5d8e782c - Fix RevisionRelationshipsTest
- 4dd7c1b1 - Fix CommentInterfaceTest
- f2a2330f - Fix CommentViewsFieldAccessTest
- bb1027ad - Fix ViewsModerationStateFilterTest
- 2c481744 - Fix ContentModerationStateTest
- dcd4e676 - Fix DefaultRevisionStateTest
- b5297813 - Fix EntityStateChangeValidationTest
- 01dd5a38 - Fix ModerationStateWidgetTest
- f6602937 - Fix ViewsModerationStateFilterTest
Toggle commit listadded 43 commits
-
d9d61601...919cebfc - 25 commits from branch
project:11.x - 919cebfc...09248172 - 8 earlier commits
- eb6407d0 - Fix WorkflowAccessControlHandlerTest
- 9f36e919 - Fix RequiredStatesTest
- 9e24da74 - Test ComplexWorkflowTypeTest
- 52c9e420 - Fix WorkflowUiTest
- fe295e95 - Fix TagTest
- 12e40d7c - Fix ViewsListTest
- 2ad5db94 - Fix ReportFieldsTest
- fd2d7366 - Fix PreviewTest
- 0f4bdceb - Fix ViewsConfigDependenciesIntegrationTest
- ffe75d64 - Fix SortTranslationTest
Toggle commit list-
d9d61601...919cebfc - 25 commits from branch
added 13 commits
- ffe75d64...874e2b8a - 3 earlier commits
- 8b2278bf - Fix EntityQueryRelationshipTest
- d2721e1b - Fix EntityViewBuilderTest
- 389beff0 - Fix ConfigEntityImportTest
- c56e8217 - Fix EntityReferenceSelectionSortTest
- a8d1db02 - Fix FileViewsAccessTest
- 78b7827b - Fix MediaTest
- bcfb5c8a - Fix MediaTypeTest
- 84b4a9d2 - Fix FilterTest
- 63bfbac9 - Fix RelatedResourceTypesTest
- 9cdd71b1 - Fix ResourceTypeRepositoryTest
Toggle commit listadded 7 commits
Toggle commit listadded 29 commits
-
aa4ed991...031f63e7 - 4 commits from branch
project:11.x - 031f63e7...9dd12460 - 15 earlier commits
- 2c04868b - Fix HandlerTest
- 0db05bb4 - Partial fix of FieldWebTest
- 69f9d456 - Fix FieldRenderedEntityTranslationTest
- c912968e - Fix FieldEntityTranslationTest
- 371769e3 - Fix UserViewsFieldAccessTest
- 6940e816 - Fix PendingRevisionTest
- 88d1eb2e - Fix VocabularySerializationTest
- f6866b0b - Fix TermParentsTest
- b9225bd2 - Fix MenuAccessControlHandlerTest
- d6e407a9 - Fix DateFormatAccessControlHandlerTest
Toggle commit list-
aa4ed991...031f63e7 - 4 commits from branch
added 1 commit
- ba1f337c - Remove default value from field_test_19 in update path fixture, since it...
added 1 commit
- 24e47d32 - Revert "Remove default value from field_test_19 in update path fixture, since...
added 5 commits
-
24e47d32...e72fa7b3 - 4 commits from branch
project:11.x - 3ca39b3f - Merge remote-tracking branch 'origin/11.x' into 3341682-label-type-validation
-
24e47d32...e72fa7b3 - 4 commits from branch
added 5 commits
-
3ca39b3f...5052fcdd - 4 commits from branch
project:11.x - fd73fea2 - Merge remote-tracking branch 'origin/11.x' into 3341682-label-type-validation
-
3ca39b3f...5052fcdd - 4 commits from branch
added 1 commit
- e09910f1 - Trigger deprecation when a view is created without a label
70 70 $this->assertValidationErrors(['editor' => "The 'non_existent' plugin does not exist."]); 71 71 } 72 72 73 /** 74 * {@inheritdoc} 75 */ 76 public function testLabelValidation(): void { 77 // @todo Remove this override in https://www.drupal.org/i/3231354. The label of Editor entities is dynamically computed: it's retrieved from the associated FilterFormat entity. That issue will change this. - Resolved by Wim Leers
- Resolved by Wim Leers
added 3 commits
- f886acfe - Revert all `views.view.*.yml` changes.
- 5e31e03b - Revert all other Views-related code changes.
- 771a1396 - Port infrastructure from https://www.drupal.org/project/drupal/issues/3364109.
added 9 commits
-
79f37e06...4c8821bc - 8 commits from branch
project:11.x - 0cd5b9a7 - Merge remote-tracking branch 'origin/11.x' into 3341682-label-type-validation
-
79f37e06...4c8821bc - 8 commits from branch
162 168 label: 'Mail' 163 169 mapping: 164 170 subject: 165 type: label 171 type: required_label Yes.
The UI simply never evolved and the config schema is from the days that nothing was required.
- This is literally the earliest config schema code, added in #1866610, in early 2013! Back then, nothing was required. Well, there was a single
required: true, but it was removed in the follow-up issue #1905230. - Why would anyone ever want to configure e-mails at
/admin/config/people/accountsthat would not have e-mail subjects? Let's investigate ️♀️
- The form logic for this lives at
AccountSettingsFormand was introduced in #1924992, which itself was not new but converted from theuser_admin_settings()function inuser.admin.inc That form logic dates back to 2007: #166742 introduced it!
-
But back then, there was fallback logic in place in
_user_mail_text(), which was removed in #1757566 (that issue converted it to the default config inuser.mail.yml)! That means that from that point onward, everything inuser.mail.ymlwas de facto required, because otherwiseuser_mail()(which still exists to this day) would fail to work correctly. In other words: the e-mail settings at/admin/config/people/accountswas de facto required, because no more fallback logic exists: the default config is the fallback, and if you set ainput[type=text]to the empty string, it's just gone.
In other words: in order to keep the behavior that was intended all along, all the way back to 2007, we need this to be required.
This is just one example, I'm sure there are dozens or hundreds more.
So: fixed that in 20e5e990.
- This is literally the earliest config schema code, added in #1866610, in early 2013! Back then, nothing was required. Well, there was a single
- Resolved by Wim Leers
378 378 label: 'Timestamp ago display format settings' 379 379 mapping: 380 380 future_format: 381 type: label 381 type: required_label 382 382 label: 'Future format' 383 383 past_format: 384 type: label 384 type: required_label Did a similar investigation here as I did for
mail:subject.Here, you're right! The key difference is that
\Drupal\Core\Field\Plugin\Field\FieldFormatter\TimestampAgoFormatter::formatTimestamp()does not read the config directly but instead calls$this->getSetting('future_format'), which results in calling this:public function getSetting($key) { // Merge defaults if we have no value for the key. if (!$this->defaultSettingsMerged && !array_key_exists($key, $this->settings)) { $this->mergeDefaults(); } return $this->settings[$key] ?? NULL; } /** * Merges default settings values into $settings. */ protected function mergeDefaults() { $this->settings += static::defaultSettings(); $this->defaultSettingsMerged = TRUE; }which means that
\Drupal\Core\Field\Plugin\Field\FieldFormatter\TimestampAgoFormatter::defaultSettings()gets called:public static function defaultSettings() { return [ 'future_format' => '@interval hence', 'past_format' => '@interval ago', 'granularity' => 2, ] + parent::defaultSettings(); }… which means everything will continue to work fine even without this value

Nope, it doesn't!

Because the merging logic only checks if the key-value pair exists. It doesn't check if it has an actual value. So that means I can today configure it like this:
I can save this and then the end result is:In other words: the current admin UI does not protect the end user!
Pushed fix in 87bdbedd.
- Resolved by Wim Leers
75 75 # @see \Drupal\views_ui\ViewAddForm::form() 76 76 max: 128 77 77 label: 78 type: label 78 type: required_label I already proposed exactly that at https://www.drupal.org/project/drupal/issues/3341682#comment-15174215.
@phenaproxima listed good reasons at https://www.drupal.org/project/drupal/issues/3341682#comment-15174615.
But … I think you're right that it'd be better to combine:
- making the label required
- deprecate the fallback logic in
View::label()(+ trigger a deprecation error: https://www.drupal.org/project/drupal/issues/3341682#comment-15174650) - updating the default views + tests
… all in a single issue: https://www.drupal.org/project/drupal/issues/3380480. That'd definitely make it a more coherent change.
So: did that in ca0cd954.
Edited by Wim Leerschanged this line in version 51 of the diff
Updated the follow-up issue: https://www.drupal.org/project/drupal/issues/3380480#comment-15195495
added 1 commit
- ca0cd954 - Move making `views.view.*:label` required to https://www.drupal.org/project/drupal/issues/3380480.


