Automatically closed - issue fixed for 2 weeks with no activity.
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
Follow-ups:
- #19.1: 📌 [later phase] [PP-1] When the field type for a PropShape changes, the Content Creator must be able to upgrade Postponed (and I realized it had a blocker itself: ✨ [later phase] PropShapes' JSON schema must match exactly with FieldType's storage format — what if you want to use another FieldType? Active ), and then the 3 at the bottom of #17:
- 📌 [PP-1] Auto-generate Component config entities for all discovered SDCs that meet XB's minimum criteria Active
- 📌 [PP-2] Remove Component config entity's add/edit form Postponed
-
📌
[PP-1] Introduce `FieldTypeStorageForJsonSchemaDefinition` plugin type
Postponed
Bonus follow-ups: 📌 Component config entity should validate that the SDC actually (still) exists Active + 📌 [PP-2] Add ComponentAuditabilityTest Active .
I'll continue working on this MR on Monday. Now: time for blog posts :)
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
Actually, the
Component
config entity already handles this, largely. So we don't need all that infrastructure.But we're missing some validation here.
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
Wow, that looks much better! 👏
I'd even use the word impressive — it's nice to see that when we use Symfony's Validation component "as intended", that it is so much clearer than if there's Drupal's layer of abstraction (either config schema or Entity/Field/Typed Data) in between 🤯
Is there any documentation about how `#config_target` should/can be used?
- 🇺🇸United States tedbow Ithaca, NY, USA
@Wim Leers I changed this to use symfony/validator constraints. I think this is better.
I still need to add some test cases for missing keys and type errors but this logic should already be covered by the constraints.
So I have still have work on it. If you want to just wait until I am done, I can just continue working on it. But if you want to look at the approach feel free
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
Just discussed #3461499-17: Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings → with @laurii, he agrees that all defaults should be specified in the SDC, not in a
Component
. See #3461499-19: Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings → and preceding comments.That means this is obsolete.
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
Discussed #17 with @lauriii. His replies:
- Yes, but needs follow-up to devise a UX to allow existing
StaticPropSource
s using a previous field type+widget to update to the new one. - Yes
- Yes
He agreed with the 3 follow-ups proposed at the bottom of #17.
We also discussed #18, and he prefers the "alternatively" — so repurposed that 👍
P.S.: Note that this also blocks 📌 Create a component for testing form backend + frontend integration Active .
- Yes, but needs follow-up to devise a UX to allow existing
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
Oh, and once this lands, 📌 [PP-1] Media Library integration Postponed becomes trivially unblocked: it'll only require:
- new configuration to allow declaring a preference for
image
vsmedia
(which oddly would not be possible through a newexperience_builder.settings.yml
config, only through a new config entity, because simple config cannot have module nor config dependencies) - updating
\Drupal\experience_builder\SdcPropJsonSchemaType::findFieldTypeStorage()
to respect that configuration
Those 2 things would be in an MR for that issue which I could start and then the front-end team could continue.
Alternatively, I could repurpose ✨ Hardcode image props to use the media widget temporarily Active for that, so that #3454173 can stay focused on the bigger picture, of making that media library widget actually work (well).
- new configuration to allow declaring a preference for
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
The
\Drupal\experience_builder\Form\ComponentEditForm
UX became simpler:Discussion with @lauriii
Yesterday morning, prior to writing #15, @lauriii and I met and discussed this problem space in detail.
The key outcomes of that discussion:
- XB will require that an SDC provides an example value for it to be surfaced in XB. IOW: the
examples
array must not be empty, and XB will use the first one. ⇒ Removes the need for 25% of what theComponent
config entity provides. - @lauriii was heavily leaning towards "no UI" and "logic with an alter hook".
- Tricky: complex shapes, like
json-schema-definitions://experience_builder.module/image
— those would need a plugin system to provide the appropriate field type + field storage.
This issue: current MR state and current consequences
This issue proves it's possible to generate the default
field_type
,expression
andfield_widget
value for each prop. ⇒ Removes the need for 75% of what theComponent
config entity provides.This issue did implement it all as logic (i.e. with nothing stored as a config entity — see my comment #15 for why that turned out to be trickier than I thought when I created this issue), with a TODO for an alter hook.
As demonstrated in #16, this means the scope for 📌 [PP-1] Add support for matching SDC prop shape: {type: string, enum: …} Postponed changes to only having to worry about matching existing field instances that can be adapted to match the SDC prop's enum values.
Questions for @lauriii prior to landing this MR
- Is it acceptable for the logic that determines which field type + widget to use to change and hence newly created instances of this component to potentially use a different field type than the pre-existing component instances?
- SDC supports default
*.component.yml
-defined values. But that won't work for e.g. a default image. Is it okay for XB to layer on support for that by auto-discovering apropname.(gif|png|jpg|…)
file in the SDC's directory? 🤔 (Will add this to 📌 [SPIKE] Comprehensive plan for integrating with SDC Needs work , to potentially add that to upstream SDC.) - Is it acceptable that the default value sourced from a component's
examples
cannot be overridden on a per-site basis?
I think the answer to the first question is "yes". (Note that every stored
StaticPropSource
is self-contained: the user will continue to be able to edit it, even if it's a completely different field type.)If the answer to the second and third questions is "yes", then we can in principle remove the
Component
config entity. But that would introduce a huge risk: it'd make exporting/sharing configuration (in the future: 🌱 [later phase] [META] 7. Content type templates — aka "default layouts" — affects the tree+props data model Active , but now:config/optional/field.field.node.article.field_xb_demo.yml
, which has config dependencies on theComponent
config entities it uses) brittle.Therefore I think it'd be preferable to keep
Component
config entities under the hood — without showing any of that in the UI. I think that would also keep the door open for supporting per-site default values in the future, plus it will likely help with 🌱 [META] Support component types other than SDC Active .If you agree, then I'll create three follow-up issues:
- one that generates
Component
config entities for all discovered SDCs 1) upon installing XB, 2) upon installing a new module or theme - one to remove
\Drupal\experience_builder\Form\ComponentEditForm
after that auto-generation is happening - one for computing the appropriate field type + storage for
$ref
-defined prop shapes
- XB will require that an SDC provides an example value for it to be surfaced in XB. IOW: the
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
I started with a test that gathers all unique SDC prop schemas. Then I started writing logic to automatically suggest the sane core-provided field type choice.
In doing so, I started treading on the territory of #3456008 — and I believe that we now should not do that issue anymore, at least not in its original scope: 📌 [PP-1] Add support for matching SDC prop shape: {type: string, enum: …} Postponed .
I am now thinking that not using a config entity type might actually be the only viable choice, because the config entity being proposed here only is viable if there is a good way to transform a given
SDC prop schema
into aPropShapeInput config entity ID
— because that's how we could achieve sane dependencies.The only reason that worked in the original proposal, is because HEAD does not yet support the
{type: string, enum: …}
use case, which is AFAICT widespread among SDCs. Once that is considered, the only way to generate a config entity ID is by hashing the enum.The issue title + summary need a drastic update. See the new test class, which has 3 test methods that build on top of each other. I'll continue this MR tomorrow and will update this issue as well as 🌱 [META] Early phase back-end work coordination Active .
Automatically closed - issue fixed for 2 weeks with no activity.
The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
- @wim-leers opened merge request.
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
@lauriii is not yet convinced, but he's not available to discuss it further, so to A) get this moving, B) ground the conversation with @lauriii more, getting started on this nonetheless.
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
This is making a pretty drastic change that is not captured in the issue summary: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... — in that comment I explain why we should revert that and leave such a change for 🌱 [META] Support component types other than SDC Active to make.
I also found 2 critical gaps in the test coverage:
- https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
- https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
… and a number of additional questions.
- 🇺🇸United States phenaproxima Massachusetts
📌 Add validation constraints to language.content_settings.*.* Needs work landed!
- 🇬🇧United Kingdom longwave UK
Committed to 11.1.x and 10.4.x, this isn't a bug fix or critical task so not eligible for backport to 11.0.x or 10.3.x
Committed and pushed ac88e9a4d8 to 11.x and 6fc96204f7 to 10.4.x. Thanks!
-
longwave →
committed ac88e9a4 on 11.x
Issue #3458321 by narendraR, smustgrave, alexpott: Add validation...
-
longwave →
committed ac88e9a4 on 11.x
-
longwave →
committed 6fc96204 on 10.4.x
Issue #3458321 by narendraR, smustgrave, alexpott: Add validation...
-
longwave →
committed 6fc96204 on 10.4.x
- 🇺🇸United States phenaproxima Massachusetts
📌 Fix Block config entity type config schema violations: weight, property Postponed is in!
-
alexpott →
committed 1c218641 on 11.x
Issue #3379725 by Wim Leers, phenaproxima, narendraR, alexpott, quietone...
-
alexpott →
committed 1c218641 on 11.x
- 🇬🇧United Kingdom alexpott 🇪🇺🌍
Committed 1c21864 and pushed to 11.x. Thanks!
Given where we are at in the 11.0.0 release cycle we can't commit this to 11.0.0 and have to target 11.1.0 instead. I've updated the deprecation messages and tests on commit.
diff --git a/core/modules/block/src/Entity/Block.php b/core/modules/block/src/Entity/Block.php index 2effc409b7..68582e48f0 100644 --- a/core/modules/block/src/Entity/Block.php +++ b/core/modules/block/src/Entity/Block.php @@ -349,7 +349,7 @@ public function preSave(EntityStorageInterface $storage) { parent::preSave($storage); if (!is_int($this->weight)) { - @trigger_error('Saving a block with a non-integer weight is deprecated in drupal:11.0.0 and removed in drupal:12.0.0. See https://www.drupal.org/node/3462474', E_USER_DEPRECATED); + @trigger_error('Saving a block with a non-integer weight is deprecated in drupal:11.1.0 and removed in drupal:12.0.0. See https://www.drupal.org/node/3462474', E_USER_DEPRECATED); $this->setWeight((int) $this->weight); } diff --git a/core/modules/block/tests/src/Kernel/BlockValidationTest.php b/core/modules/block/tests/src/Kernel/BlockValidationTest.php index 9a1a2412d6..2456e7fb57 100644 --- a/core/modules/block/tests/src/Kernel/BlockValidationTest.php +++ b/core/modules/block/tests/src/Kernel/BlockValidationTest.php @@ -176,7 +176,7 @@ public function testWeightValidation(): void { public function testWeightCannotBeNull(): void { $this->entity->set('weight', NULL); $this->assertNull($this->entity->getWeight()); - $this->expectDeprecation('Saving a block with a non-integer weight is deprecated in drupal:11.0.0 and removed in drupal:12.0.0. See https://www.drupal.org/node/3462474'); + $this->expectDeprecation('Saving a block with a non-integer weight is deprecated in drupal:11.1.0 and removed in drupal:12.0.0. See https://www.drupal.org/node/3462474'); $this->entity->save(); } diff --git a/core/modules/search/search.module b/core/modules/search/search.module index 8d5b1e330d..0944c53dcf 100644 --- a/core/modules/search/search.module +++ b/core/modules/search/search.module @@ -428,7 +428,7 @@ function search_block_presave(BlockInterface $block) { if ($block->getPluginId() === 'search_form_block') { $settings = $block->get('settings'); if ($settings['page_id'] === '') { - @trigger_error('Saving a search block with an empty page ID is deprecated in drupal:11.0.0 and removed in drupal:12.0.0. To use the default search page, use NULL. See https://www.drupal.org/node/3463132', E_USER_DEPRECATED); + @trigger_error('Saving a search block with an empty page ID is deprecated in drupal:11.1.0 and removed in drupal:12.0.0. To use the default search page, use NULL. See https://www.drupal.org/node/3463132', E_USER_DEPRECATED); $settings['page_id'] = NULL; $block->set('settings', $settings); }
- 🇧🇪Belgium borisson_ Mechelen, 🇧🇪
I reviewed it again today as well, and the one remark I had was answered. RTBC++
- 🇺🇸United States phenaproxima Massachusetts
Okay, I agree with you @tim.plunkett; did the deprecation dance around page_id and added a change record.