Skip to content
Snippets Groups Projects

Issue #3341682: New config schema data type: `required_label`

Closed Issue #3341682: New config schema data type: `required_label`
6 open threads

Merge request reports

Approval is optional

Closed by Lauri TimmaneeLauri Timmanee Aug 17, 2023 (Aug 17, 2023 9:29am UTC)

Merge details

  • The changes were not merged into 11.x.

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Adam G-H
  • 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 }
  • Adam G-H
  • Adam G-H
  • 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 }
    • Comment on lines +199 to +201

      I don't understand this. If there's not a label key, we want to test the removal of labels...??

    • Author Developer

      Yes. See the one test that uses this: Block. That config entity type stores its label not as a top-level property, but as a second level property, which is why it is impossible to define that as an entity key.

      :shrug:

    • Author Developer

      This was simplified months ago. :nerd:

    • Adam G-H changed this line in version 41 of the diff

      changed this line in version 41 of the diff

    • Please register or sign in to reply
  • Adam G-H
  • Adam G-H
  • Wim Leers added 4 commits

    added 4 commits

    Compare with previous version

  • Wim Leers added 1 commit

    added 1 commit

    Compare with previous version

  • Wim Leers added 1 commit

    added 1 commit

    Compare with previous version

  • Wim Leers added 219 commits

    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

    Compare with previous version

  • Wim Leers added 1 commit

    added 1 commit

    • 7b3fc9cc - Convert all uses of `{ type: string, translatable: true }` to `{ type: translatable_string }`.

    Compare with previous version

  • Wim Leers added 67 commits

    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

    Compare with previous version

  • Wim Leers added 1 commit

    added 1 commit

    • 872ea10e - @alexpott review: no more `translatable_string`, `required_label` instead.

    Compare with previous version

  • Wim Leers added 3 commits

    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.

    Compare with previous version

  • Lauri Timmanee changed the description

    changed the description

  • Lauri Timmanee changed target branch from 10.1.x to 11.x

    changed target branch from 10.1.x to 11.x

  • Wim Leers added 338 commits

    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

    Compare with previous version

  • Wim Leers added 1 commit

    added 1 commit

    • d70d7845 - Drupal 10.2 no longer allows the word "please" :shrug:‍♂️

    Compare with previous version

  • Wim Leers added 2 commits

    added 2 commits

    • 0dc00a05 - Restore `translatable: true` for `type: label`.
    • db98ea65 - Update `::testLabelValidation()` after #3349293 landed.

    Compare with previous version

  • Wim Leers added 2 commits

    added 2 commits

    • 9ac54d94 - Update expected type for config entity type label in `ConfigSchemaTest`.
    • 40edf8ba - Convert the other config entity type labels to use `type: required_label`.

    Compare with previous version

  • Wim Leers added 2 commits

    added 2 commits

    • ae69e0e8 - Remove obsolete change.
    • db1d7b89 - Moved to #3377030: `type: label` -> `type: text` conversion now considered out of scope.

    Compare with previous version

  • Wim Leers changed title from Issue #3341682: Add config validation to the "label" config schema data type to Issue #3341682: New config schema data type{+: required_label+}

    changed title from Issue #3341682: Add config validation to the "label" config schema data type to Issue #3341682: New config schema data type{+: required_label+}

  • Wim Leers added 7 commits

    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

    Compare with previous version

  • Wim Leers added 2 commits

    added 2 commits

    • f0ca98b4 - When creating Action entities and no label is specified, fall back to the action plugin label.
    • 6645787b - `NodeType` has no `label` property, but a `name` property that is the label.

    Compare with previous version

  • Wim Leers added 5 commits

    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.

    Compare with previous version

  • Adam G-H added 1 commit

    added 1 commit

    • d2d62214 - Add section labels in demo_umami page layouts

    Compare with previous version

  • Adam G-H added 2 commits

    added 2 commits

    • f332acfe - Revert "Add section labels in demo_umami page layouts"
    • 24ed6c4a - Revert a couple of required_label uses in places where it does not make as much sense

    Compare with previous version

  • Adam G-H added 10 commits

    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

    Compare with previous version

  • Adam G-H added 4 commits

    added 4 commits

    • 44da76db - Fix coding standards
    • d2aa8c8b - Fix DisplayModeUpdateTest
    • 92f361dc - Fix DisplayApiTest
    • f400adf1 - That was the MOST TEDIOUS FUCKING THING I have done in a long time

    Compare with previous version

  • Adam G-H added 1 commit

    added 1 commit

    Compare with previous version

  • Adam G-H added 43 commits

    added 43 commits

    Compare with previous version

  • Adam G-H added 13 commits

    added 13 commits

    Compare with previous version

  • Adam G-H added 7 commits

    added 7 commits

    Compare with previous version

  • Adam G-H added 29 commits

    added 29 commits

    Compare with previous version

  • Adam G-H added 5 commits

    added 5 commits

    • c71ef758 - Fix D7 MigrateSearchPageTest for real this time
    • fb02df48 - Fix UserAdminSettingsFormTest
    • 213a08e4 - Fix ViewsFixRevisionIdUpdateTest
    • 0dea6741 - Fix BaseFieldOverrideTest
    • 9f7af3f9 - Hopefully fix MediaTest

    Compare with previous version

  • Adam G-H added 1 commit

    added 1 commit

    • ba1f337c - Remove default value from field_test_19 in update path fixture, since it...

    Compare with previous version

  • Adam G-H added 1 commit

    added 1 commit

    • 24e47d32 - Revert "Remove default value from field_test_19 in update path fixture, since...

    Compare with previous version

  • Adam G-H added 5 commits

    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

    Compare with previous version

  • Adam G-H added 5 commits

    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

    Compare with previous version

  • Adam G-H added 1 commit

    added 1 commit

    • e09910f1 - Trigger deprecation when a view is created without a label

    Compare with previous version

  • Adam G-H added 1 commit

    added 1 commit

    Compare with previous version

  • 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.
  • Joris Vercammen
  • Joris Vercammen
  • Wim Leers added 3 commits

    added 3 commits

    Compare with previous version

  • Wim Leers added 1 commit

    added 1 commit

    • 79f37e06 - Revert all `SearchPage` modifications.

    Compare with previous version

  • Wim Leers added 9 commits

    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

    Compare with previous version

  • 162 168 label: 'Mail'
    163 169 mapping:
    164 170 subject:
    165 type: label
    171 type: required_label
    • The UI doesn't actually enforce these as required labels. Are these really required?

    • Author Developer

      Yes.

      The UI simply never evolved and the config schema is from the days that nothing was required.

      1. 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.
      2. Why would anyone ever want to configure e-mails at /admin/config/people/accounts that would not have e-mail subjects? Let's investigate :spy:️‍♀️
      3. The form logic for this lives at AccountSettingsForm and was introduced in #1924992, which itself was not new but converted from the user_admin_settings() function in user.admin.inc :older_man: That form logic dates back to 2007: #166742 introduced it!
      4. 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 in user.mail.yml)! That means that from that point onward, everything in user.mail.yml was de facto required, because otherwise user_mail() (which still exists to this day) would fail to work correctly. In other words: the e-mail settings at /admin/config/people/accounts was de facto required, because no more fallback logic exists: the default config is the fallback, and if you set a input[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.

    • Please register or sign in to reply
  • Lauri Timmanee
  • 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
    • The UI doesn't actually enforce this as a required label. Is this really required?

    • Author Developer

      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 :thumbsup:

      :flushed: Nope, it doesn't! :see_no_evil:

      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:

      Screenshot 2023-08-16 at 11.19.30 AM.pngI can save this and then the end result is:

      Screenshot 2023-08-16 at 11.24.08 AM.png

      In other words: the current admin UI does not protect the end user!

      Pushed fix in 87bdbedd.

    • Please register or sign in to reply
  • Lauri Timmanee
  • Adam G-H added 3 commits

    added 3 commits

    • 337c7d96 - Merge remote-tracking branch 'origin/11.x' into 3341682-label-type-validation
    • a281e841 - Merge branch '3341682-label-type-validation' of...
    • bb2de851 - Link to todos

    Compare with previous version

  • Wim Leers added 1 commit

    added 1 commit

    Compare with previous version

  • Wim Leers added 2 commits

    added 2 commits

    • 20e5e990 - Make all the "subject" inputs for the "Emails" vertical tabs in...
    • 87bdbedd - Make `future_format` and `past_format` required in `TimestampAgoFormatter::settingsForm()`.

    Compare with previous version

  • 75 75 # @see \Drupal\views_ui\ViewAddForm::form()
    76 76 max: 128
    77 77 label:
    78 type: label
    78 type: required_label
  • Wim Leers added 1 commit

    added 1 commit

    Compare with previous version

  • Please register or sign in to reply
    Loading