- Issue created by @wim leers
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Here's a patch that updates the existing test coverage to use the
text
field type instead of thestring
field type and it does not cause a test failure. That proves thatComponent::calculateDependencies()
is missing some additional logic.As the issue summary says: Look at how Drupal's existing Field UI handles this.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
FYI: I marked this for a later phase because this is IMHO low-risk. It's a known TODO, with a known solution, in Drupal core.
Sibling follow-up issue 📌 [PP-1] Default props values should support files/images Postponed is much riskier, so is not marked for a later phase.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Sibling issue: 📌 `StringSemanticsConstraint::MARKUP`: agree how SDC prop JSON schema can convey it should be markup, and Needs review .
- Status changed to Active
2 months ago 1:34pm 18 March 2025 - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
If #3467959-35: SDC and code component props should be able to receive HTML, editable in CKEditor 5 → is considered acceptable, we won't have to deal with the
FilterFormat
scenario described in this issue.But … the root problem remains. See the example of XB's
image
SDC:uuid: 1574ac0a-b1b6-4b32-aaef-a15c18d95aa0 langcode: en status: true dependencies: module: - media_library label: Image id: sdc.experience_builder.image provider: experience_builder source: sdc category: Other settings: plugin_id: 'experience_builder:image' prop_field_definitions: image: field_type: entity_reference field_storage_settings: target_type: media field_instance_settings: handler: 'default:media' handler_settings: target_bundles: image: image field_widget: media_library_widget default_value: { } expression: 'ℹ︎entity_reference␟{src↝entity␜␜entity:media:image␝field_media_image␞␟entity␜␜entity:file␝uri␞␟url,alt↝entity␜␜entity:media:image␝field_media_image␞␟alt,width↝entity␜␜entity:media:image␝field_media_image␞␟width,height↝entity␜␜entity:media:image␝field_media_image␞␟height}'
… while this contains
dependencies: module: - media_library
That is inadequate, it should have:
dependencies: config: - media.type.image module: - media_library
(due to its
field_instance_settings
)IOW:
\Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase::calculateDependencies()
must be updated. - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
📌 Calculate field and component dependencies on save and store them in an easy to retrieve format Active is making this a lot simpler.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
📌 Calculate field and component dependencies on save and store them in an easy to retrieve format Active is in, I'll need to double-check what remains here.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
This is still necessary.
The root cause is
StaticPropSource::calculateDependencies()
:/** * {@inheritdoc} */ public function calculateDependencies(FieldableEntityInterface|FieldItemListInterface|null $host_entity = NULL): array { assert($host_entity === NULL || $host_entity instanceof FieldableEntityInterface); // The only dependencies are those of the used expression. If a host entity // is given, then `content` dependencies may appear as well; otherwise the // calculated dependencies will be limited to the entity types, bundle (if // any) and fields (if any) that this expression depends on. // @see \Drupal\Tests\experience_builder\Kernel\PropExpressionDependenciesTest return $this->expression->calculateDependencies($this->fieldItemList); }
… this is only looking at the
field_type
s involved in the expression.We need this to be expanded, to do something like
\Drupal\Core\Field\FieldConfigBase::calculateDependencies()
, which does:// Let the field type plugin specify its own dependencies. // @see \Drupal\Core\Field\FieldItemInterface::calculateDependencies() $this->addDependencies($definition['class']::calculateDependencies($this));
… which would call
\Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem::calculateDependencies()
and hence in turn solve this. - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Still needs expanded test coverage.
AFAICT this will need a new
ComponentSourceInterface::calculateSettingsDependencies()
method; akin to\Drupal\Core\Field\FieldItemInterface::calculateStorageDependencies()
. - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
wim leers → changed the visibility of the branch 0.x to hidden.
- First commit to issue fork.
- Assigned to isholgueras
- Status changed to Needs work
12 days ago 7:33pm 19 May 2025 - First commit to issue fork.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
While this could theoretically be retroactively fixed, this is the last "missing dependency" issue to my knowledge! So with this in done, XB's usage information would actually be completely accurate, and could prevent painful surprises/brokenness (by preventing uninstallation).
Also: 📌 Version component prop definitions for SDC and Code components Active will quite cause not 1 but multiple versions to exist of the
prop_field_definitions
. So: it'd be good to have this fixed before that, to ensure nothing is overlooked.So: tagging .
- 🇺🇸United States tedbow Ithaca, NY, USA
Do we need to update `\Drupal\experience_builder\PropSource\DynamicPropSource::calculateDependencies` or create another issue for that? Maybe this will become obvious as I review this more. But thought I would put it here not to forgot or if someone else knows
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
#26: no, that is not necessary. I see that
PropSourceTest::testDynamicPropSource()
doesn't have an explicit example, butPropExpressionDependenciesTest::testCalculateDependencies()
does already test that e.g. aDynamicPropSource
using:ℹ︎␜entity:node:article␝field_image␞␟value
results in a calculated dependency onfield.field.node.article.field_image
ℹ︎␜entity:node:article␝field_image␞␟{src↝entity␜␜entity:file␝uri␞␟url,width↠width}
also results in a calculated dependency onfield.field.node.article.field_image
- Big thanks to @tedbow for carefully reviewing @nacho's refactor towards a data provider! 🙏
- Follow-up created for (pre-existing) bug surfaced in this MR's test coverage:
🐛
SdcPropKeysConstraintValidator::validate() should complain about extraneous keys too, not just missing keys
Active
.
- My proposed resolution in #19 was pointing to the right thing, but was overkill, which @isholgueras identified 👍 No interface expansion, just an addition to
StaticPropSource
🥳
-
wim leers →
committed 89e64edc on 0.x
Issue #3460230 by isholgueras, wim leers, tedbow, catch: Component...
-
wim leers →
committed 89e64edc on 0.x