Problem/Motivation

The new method added by #2392057: Config schema fails to expand dynamic top-level types has a small but nasty bug.

Instead of adding a definition for the computed type, it replaces the original definition. That means the second and following elements for the same wildcard/generic type use the definition for the first and fail.

Proposed resolution

Use the correct key to set it.

Part of the problem seems to be the confusing naming between $type and $base_plugin_id. $base_plugin_id isn't "base", it's the real, full, replaced type. We could switch the variables to make the code easier to understand.

Remaining tasks

User interface changes

API changes

Data model changes

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Berdir created an issue. See original summary.

Berdir’s picture

Status: Active » Needs review
FileSize
647 bytes

Status: Needs review » Needs work

The last submitted patch, 2: config-replacements-cache-2663410-2.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.13 KB

Here's a test that (i think) shows we have a problem as describe by @Berdir.

alexpott’s picture

The patch attached just doesn't dynamically cache if we have a sub type

alexpott’s picture

Hmmm actually $sub_type is problematic we're actually still working out the type.

The last submitted patch, 4: 2663410-4.patch, failed testing.

alexpott’s picture

@Berdir that the type was not correct - in order for it to be meaningful we need to combine both $type and $sub_type. I've used || as the separator because it is not used in schema yet.

The test in #4 failed as expected.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Can confirm that the latest patch works for my page_manager page variant example. Since nothing is left of my initial patch I think I can RTBC this, although someone else confirming wouldn't hurt.

  • catch committed 205f549 on 8.1.x
    Issue #2663410 by alexpott, Berdir: TypedConfigManager::...
catch’s picture

Status: Reviewed & tested by the community » Fixed

This looks good to me as well. Committed/pushed to 8.1.x, thanks!

Status: Fixed » Closed (fixed)

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

Berdir’s picture

Version: 8.1.x-dev » 8.0.x-dev
Status: Closed (fixed) » Needs review

Should we also add this to 8.0.x? The issue that this fixes also went into 8.0.x, so this is currently still broken there.

Berdir’s picture

Reuploading the patch.

yanniboi’s picture

Status: Needs review » Needs work

This isn't quite finished, because the $sub_type isn't being built fully...

yanniboi’s picture

Version: 8.0.x-dev » 8.1.x-dev
FileSize
1.07 KB

Does this make sense?

yanniboi’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 16: 2663410-15.patch, failed testing.

yanniboi’s picture

Status: Needs work » Needs review
FileSize
1.07 KB

Dam I uploaded the last patch against 8.0.x not 8.1.x. Sorry....

Berdir’s picture

This works fine for me, if you still see a problem then that might be better as a separate issue, with steps to reproduce.

The last submitted patch, 16: 2663410-15.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 19: 2663410-15.patch, failed testing.

The last submitted patch, 16: 2663410-15.patch, failed testing.

yanniboi’s picture

Version: 8.1.x-dev » 8.0.x-dev
Status: Needs work » Needs review
Related issues: +#2693081: Merging data types leads to incomplete definition

sorry @Berdir I shouldn't have just jumped on this issue like that, I was half way through posting a separate issue when @timplunkett pointed me at this issue. I have created a follow up issue to explain what is not working for me:

From issue summary:

Instead of adding a definition for the computed type, it replaces the original definition. That means the second and following elements for the same wildcard/generic type use the definition for the first and fail

With patch from #8 the second definition is no longer replacing the first, however the second definition isn't fully built. The second definition does not inherit any mappings from its parent definitions.

The way I came across this was working with page_manager and ctools submodule ctools_views:

block.settings.views_block:* is of type parent definition type views_block which provides mappings for 'views_label' and 'items_per_page'. When adding a views block to a page_manager page, the resulting config object has a definition of ctools.block_plugin.*||block.settings.views_block:* and was inheriting all of the correct mappings from ctools.block_plugin.* (eg. 'region', 'weight', etc.) but none of the mappings for block.settings.views_block:*.

This is because when the definitions are merged, it currently only looks at block.settings.views_block:* and not its parents:

  • block.settings.views_block:*
  • views_block
  • block_settings
  • etc.

I have a patch to fix this here: #2693081: Merging data types leads to incomplete definition

Sorry again for disrupting this issue.

alexpott’s picture

@yanniboi did you mean to re-close this issue in #24?

Berdir’s picture

No, I reopened this in #13/14 to also get this into 8.0.x since we only committed this to 8.1.x without a good reason as far as I see (the referenced issue did get into 8.0.x..).

Soon no longer worth the trouble, not sure anymore at this point.

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.

  • catch committed 205f549 on 8.3.x
    Issue #2663410 by alexpott, Berdir: TypedConfigManager::...

  • catch committed 205f549 on 8.3.x
    Issue #2663410 by alexpott, Berdir: TypedConfigManager::...

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.

  • catch committed 205f549 on 8.4.x
    Issue #2663410 by alexpott, Berdir: TypedConfigManager::...

  • catch committed 205f549 on 8.4.x
    Issue #2663410 by alexpott, Berdir: TypedConfigManager::...

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.

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

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

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

jhedstrom’s picture

Does this still need to be open? Looks like it's been committed to 8.4 and 8.3.

Berdir’s picture

Status: Needs review » Fixed

I guess we can close this, yes.

Status: Fixed » Closed (fixed)

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