Updated: Comment #16

Problem/Motivation

We need to be able to have dots after the config prefix in config entities. This is used by fields, entity form and display modes extensively. And this works with config schema so I don't think we should be completely removing the 'config_test.dynamic.dotted.default' as this is testing that we support their use case. But the issue with the config_test.dynamic entity type is that it has ids with and without a dot. What the config system can not support is this inconsistency.

Proposed resolution

  • Remove the ability for block machine names to have a dot
  • Remove the ability for entity display and form mode machine names to have aditional dots
  • Change all usages of the config_test.dynamic entity type to have an id with a dot

I think this might have exposed a bug since being able to add a dot into entity display and form mode machine names seems brittle to me as the dot is used to delimit entity types and bundles in CMI files when the modes are used.

Remaining tasks

Possibly discuss whether we should introduce some sort of automated validation regex on a config entity's annotation?

User interface changes

None.

API changes

None so far.

#1952394: Add configuration translation user interface module in core

Files: 
CommentFileSizeAuthor
#26 2100203-config-entity-dot-26.patch13.26 KBvijaycs85
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,122 pass(es), 113 fail(s), and 15 exception(s). View

Comments

tim.plunkett’s picture

Component: base system » configuration entity system
Issue tags: +Configuration system, +Configurables

Tags

Gábor Hojtsy’s picture

Priority: Normal » Major
Issue tags: +D8MI, +language-config

This is at least major if not critical. Users can create machine names for config entities that will not match the config schemas anymore. That is kind of problematic.

alexpott’s picture

Status: Active » Needs review
FileSize
17.94 KB
FAILED: [[SimpleTest]]: [MySQL] 59,245 pass(es), 5 fail(s), and 0 exception(s). View

Here's an initial patch to see what breaks. We do need to be able to have dots after the config prefix in config entities. This is used by fields, entity form and display modes extensively. And this works with config schema so I don't think we should be completely removing the 'config_test.dynamic.dotted.default' as this is testing that we support their use case. But the issue with the config_test.dynamic entity type is that it has ids with and without a dot. What the config system can not support is this inconsistency.

The patch:

  • Renames all the blocks provided by profiles to not have a dot
  • Removes the ability for block machine names to have a dot
  • Removes the ability for entity display and form mode machine names to have a dot
  • Changes all usages of the config_test.dynamic entity type to have an id with a dot

I think this might have exposed a bug since being able to add a dot into entity display and form mode machine names seems brittle to me as the dot is used to delimit entity types and bundles in CMI files when the modes are used.

alexpott’s picture

If we want to add validation to config entity ids then I think we need to consider adding a key to the config entity type annotation that allows us to configure this.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

The patch and the explanation from #3 make total sense. And moving the logic from form handlers to the entity class for entity displays is a nice touch as well :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2100203.3.patch, failed testing.

tim.plunkett’s picture

alexpott’s picture

Status: Needs work » Postponed
alexpott’s picture

alexpott’s picture

FileSize
13.58 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2100203.10.patch. Unable to apply patch. See the log in the details link for more information. View

So now that's landed here is the patch without the profile configuration changes. The changes to what is allowed in a block machine name are still needed.

alexpott’s picture

Status: Needs work » Needs review

Go bot go

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Postponed
tim.plunkett’s picture

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImportUITest.php
@@ -37,7 +37,7 @@ function setUp() {
-    $dynamic_name = 'config_test.dynamic.new';
+    $dynamic_name = 'config_test.dynamic.new.test';

These changes are confusing, isn't the goal of this issue to do the opposite of this?

Gábor Hojtsy’s picture

Status: Postponed » Needs review
alexpott’s picture

Title: Remove ability to have dots in machine names » Make config entities use dots in machine names consistently

Updating title to reflect the nature of the change.

alexpott’s picture

Issue summary: View changes

Update summary.

alexpott’s picture

#10: 2100203.10.patch queued for re-testing.

alexpott’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue tags: +Naming things is hard
sun’s picture

I was also confused by #14, but @alexpott actually gave an explanation in #3.

I wonder whether this explanation should be moved into code comments of the relevant YAML files + test code, so that the info is not buried in an arbitrary d.o issue?

andypost’s picture

Also this could help comment module that have ugly implementation of "{comment}.field_id" ({entity_type_ID}__{bundle}) because $field->id() contains dots: #2149859: Convert the 'field_id' base field on comment entities to an entity reference field
Also this is a serious help for #2182239: Improve ContentEntityBase::id() for better DX

anavarre queued 10: 2100203.10.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 10: 2100203.10.patch, failed testing.

yched’s picture

Subscribing and pinging from #1977206: Default serialization of ConfigEntities - patch over there serializes a config entity using toArray(), which now relies on config schemas.

We currently have no clue about "the structure of IDs for a config entity type", and thus cannot find the config schema entry (with the "correct number of stars") for an entity that doesn't have a proper ID yet - example : a fresh entity being created for a "create" form. Calling toArray() on such an entity fails with a fatal.

It seems absurd that we can't access the schema without an actual entity ID. The schema is inherent to the entity type, not to the state or content of a specific entity.

vijaycs85’s picture

adding missing tags from #17

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
13.26 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,122 pass(es), 113 fail(s), and 15 exception(s). View

re-roll of #10.

Status: Needs review » Needs work

The last submitted patch, 26: 2100203-config-entity-dot-26.patch, failed testing.

alexpott’s picture

Needs a beta evaluation

alexpott’s picture

I knew that eventually not being consistent here would get us into trouble - see #2605042: Cannot edit actions created by user_user_role_insert()

effulgentsia’s picture

Issue tags: +rc target triage

I think this might have exposed a bug since being able to add a dot into entity display and form mode machine names seems brittle to me as the dot is used to delimit entity types and bundles in CMI files when the modes are used.

I haven't checked to see if this (specifically display and form modes containing dots leading to bugs elsewhere) is still an issue. In case it is, tagging this for RC target triage. If it is not, I don't know that there's anything left here that would be eligible for 8.x vs. postponing to 9.x.

xjm’s picture

Issue tags: -rc target triage

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.