smustgrave → credited fago → .
I've updated the proposal in the issue summary to reflect this is released now + worked in my proposal from the previous comment.
What changes are you missing?
When we keep the parameter, there is no change necessary to ContentEntityBase + I argued why we don't to extend unit tests here. I don't see any other changes.
> And needs to be properly be deprecated with a trigger_error and CR
Not sure, this is helpful / needed in this case. Since the parameter was already doing nothing before, there is no change and no possible breakage to bug the developer about.
But if preferred, I can add it when a FALSE or NULL value is passed instead of the TRUE default.
bump. this is filling the logs. is no one else running into this?
I agree it should be deprecated for now, not just removed. Some child implementations might be using it, so silently removing the parameter would render the child implements having a weird undocumented $notify parameter. Besides ContentEntityBase I see it implemented in \Drupal\simple_oauth\Authentication\TokenAuthUser::set
I'd argue the change needs no unit tests, even when removing the parameter, there is 0 change in behavior.
I added a MR for deprecating it.
Sure! Updated it.
I created a quick fix for that. I fixed it for all the tests where I found the issue (checking various constraint validator tests).
Disclosure: I used AI to help me with this.
jjchinquist → credited fago → .
this is about this comment: https://www.drupal.org/project/drupal/issues/2142991#comment-8309595 →
The local variable $this->typedData is named very confusingly and should be $this->typedDataManager. Those couple of tests have this issue, no biggie at all, but makes sense to fix.
almost, 📌 Avoid dynamically defining constraints Postponed: needs info is the only still be left todo. That said, I don't think we need this META anymore, the remaining single todo can stay open. Let's close it.
Checking on this, it's still relevant.
The metadata (=field definitions) is missing out on constraints that are defined via \Drupal\Core\Field\FieldItemList::getConstraints(). Some modules and even core use this to define additional constraints, like \Drupal\Core\Field\EntityReferenceFieldItemList::getConstraints. While this code does not make use of the field value, it could, but that's not intended. It would still make sense to improve this API to make sure constraints are defined not dynamically, e.g. by replacing the method with a static one - like \Drupal\Core\Field\FieldItemList::processDefaultValue.
This is of course an API change, so would need BC and communicated deprecation.
While the API change would be happening on typed data level, the impact would be mostly for field type developers. So moving this to entity field api to get some feedback of entity system maintainers.
Proposal:
- Deprecate TypedDataInterface::getConstraints()
- Add "static public function getValidationConstraints(DataDefinitionInterface $definition)"
So that means all the implementations of FieldItem::getConstraints() would have to follow and implement the static getValidationConstraints instead.
So moving into the entity queue for sub-system maintainer review.
I do think people already are used to this now.
The way I'd see it a field is just a special kind of property, and with that the loadByProperties() method makes sense for fields also. However still, I think some people are confusing by loadByProperties() and do not think they can use it for fields also.
That said, I do think it still makes sense to clarify this better and e.g. just add an alias loadByFieldValues() in ContentEntityStorage. But that will be up to the new entity maintainers to decide. So setting the right tag.
I think major is ok, it's a quite important aspect still, so leaving the priority as it is.
Fixing it on entity-access layer makes sense, but it's not solving the issue I reported here. The UI already has no way to delete filter formats, it can only be disabled. This issue is about unexpected behavior when you run the code, with drush, or via an (post)-update hook/function, that does not matter:
\Drupal\filter\Entity\FilterFormat::load('restricted_html')->delete();
So when the filter format is still referenced in field config, those fields get deleted, including the content. I'd argue this is very un-intuitive behavior, since this is not visible or clear from the code executed.
Indeed, deleting the filter-format might result in data-integrity issues. But I'd argue that's an intuitive. possible consequence from deleting a filter format manually like this, but loosing not touched field-configuration including the deletion of if its data is not.
Additionally, you can already delete filter formats that are in use with code snippets like this - so that "data integrity" issue when deleting filter formats via code is already pre-existing. The described problem only applies if the format is referenced as a "allowed-filter-format" in the field. However, if no filter formats are referenced, filter-format usage is not restricted (so it might be in use) and deleting a filter-format would not delete any fields. Thus, this "data-integrity issue" is not new, but the suggested change makes the behavior consistent in both cases.
I must say, I don't see an issue with the "data-integrity" problem, as mentioned, that's intuitive to me when you delete a filter format like this. However, if we decide it is an issue that shall be tackled instead, we should also cover the case of un-referenced filter-formats which are currently not referenced in field configs at all. That sounds like a quite complicated problem to solve though.
#3537942 got merged, thus I've update the MR. Now it can be reviewed easier.
Thx merged I'm going to rebase the other MR then.
> The only remark that needs another look is from the .module file (template suggestions but the only template there currently exists is for renderless-container).
Yes, it generally allows you to add a template for special custom-element, e.g. you could add "custom-element-drupal-markup" if somehow you want to customize it. I think that makes sense and comes with no down-side compared to only adding a single suggestion.
ready. this is based upon
📌
Integrate experience builder with entity-custom-elements-generation
Active
.
disclosure: I've leveraged AI to create the MR.
ok, new MR is ready with a couple of improvements/fixes:
- adds new renderless-container for better wrapping of multiple children, as described above
- adds template-suggestion for overriding markup output by tag
- implements renderless-container in both markup and json serialization + adds test-coverage for that
- improves XB field-formatter to avoid nesting, XB-fields are single-valued
- improves XB-field-formatter test to assert the flat structure when renering multiple components
I figured the resulting structure is weirdly nested though, this needs improvement. I'll take a stab on it before merging this.
I figured we need a way to make the normalize flatten a custom-element with slot-content in a way that the custom-element turns into a list of custom element elements. Without that it would become tedious to flatten things, since do not want to have a helper like convertRenderArray() to return null|¢ustomElement|CustomElement[]. We should be able to organize a list of custom elments in a single object, that helps to keep code clean.
Thus I'm taking a stab on adding this here. I'd call it "renderless-container", similar to the concept of "renderless-components" in the frontend world. This is not very nice, but expressfull, so when you run into it it should be clear what it is. Since the element is not rendered, we cannot really render the attributes either, so the result is that it only renders the children/slots.
Merged into 3.x-dev! Next step: 📌 Integrate experience builder with entity-custom-elements-generation Active
I do think the best and right now also the easiest way to do this is by adding a new, dedicated ce-field formatter.
The frontend can take care of showing the XB-content as a full-page and deal with other fields as needed.
This goes inline with the layout-builder integration which we recently improved to have fields next to the layout, so just keeping the output as yet another field makes sense.
I'd not use the "auto" processing to support, with the formatter we are more flexible when we need settings in the future + the existing "auto" output might be an interesting alternative for some folks, it's a verbatim output of the internally stored structure, let's better keep that there.
What probably makes sense is that we take care of auto-configuring the formatter properly in the future, so the auto-configuration based upon the field-type is something we might want to look into, but that's a generic improvement, and not in scope of this task.
Oh I see, I'm sry to have caused this issue and it ate your time. The 1.x/0.x separation was new to me. :-/
finally got it green on CI also. Getting CI work was a bit tricky since experience_builder only works with Drupal >11.2 now.
Disclosure: I've used AI (Claude Code) to help coding this, in particular the test case. I worked over all non-test code and reviewed all code changes of course.
As a preparation for figuring out how to integrate best, here is an AI-based summary of how XB takes over entity generation.
I've added a MR with test-coverage which successfully renders the XB output into a tree of custom elements.
It's for sure not perfect, but it should lay the foundation on how we do it, so we can improve the output as later step.
Next, we need to integrate this into the entity rendering pipeline either via auto-processing, a dedicated formatter, or global detection in the entity ce-display.
Thanks for the quick reviews!
It's not that we specified the wrong property, it's that the inherited \Drupal\Core\Field\FieldItemBase::mainPropertyName() does not make sense for XB's field type.
yep, exactly!
>here was 1 commit, now there's 8?!
They were there from the beginning.
Maybe, there was some glitch between 0.x and 1.x when I created the PR, I was confused about the branches. But the diff is correct, so it should be fine to just squash-merge it.
This actually concerns text fields provided by text module.
Thank you. I added the test case, verified it fails first, and then put it on top of your changes, to see it pass! All good, this seems ready then!
I also ran into this and opened 📌 Deleting filter formats may result in data loss Active for issue I faced as consequence: deletion of text item fields that have the text format as "allowed_format". I do think we should fix both, this issue and 📌 Deleting filter formats may result in data loss Active .
Checking on the proposed solutions I think both are viable. I agree that MR 12404 is the best approach though. Since it's a generally new feature, I guess it's best done over at [#579743]?
With onDependencyRemoval() we could remove the dependency and let the field and data survive, what I think would be acceptable given the deletion.
I'd prefer to deny the deletion when the field has data though, but that does not seem to be properly doable. The only documented exception for ConfigEntity:delete() is EntityStorageException what seems not suiting.
The code generating the dependency on filter-formats in text-fields is that method:
/**
* {@inheritdoc}
*/
public static function calculateDependencies(FieldDefinitionInterface $field_definition) {
// Add explicitly allowed formats as config dependencies.
$format_dependencies = [];
$dependencies = parent::calculateDependencies($field_definition);
if (!is_null($field_definition->getSetting('allowed_formats'))) {
$format_dependencies = array_map(function (string $format_id) {
return 'filter.format.' . $format_id;
}, $field_definition->getSetting('allowed_formats'));
}
$config = $dependencies['config'] ?? [];
$dependencies['config'] = array_merge($config, $format_dependencies);
return $dependencies;
}
As seen, it only adds dependencies for the "allowed_formats". This seems bogus, since there is no larger harm when a filter-format this is a allowed-format is removed than when a filter-format that is not referenced is removed. When all filter-formats are allowed, no dependency is added, the filter format might be used, and it can be deleted. What happens is that the field and its data is not removed, it just gets a reference on a selected filter format.
On the other hand when a filter format is removed that is part of the "allowed-formats", since there is just one entry less that users can select. The more severe data integrity issue is the same as before.
That said, the right fix here is probably to add a config dependency on all filter-formats when the list of filter-formats is not restricted.
Then, given that we already have the issue that filter formats leads to data with invalid text-format-references when no "allowed_formats" are set, implementing onDependencyRemoval() to avoid field-data deletion in case of "allowed_formats" usage seems reasonable.
Thank you Kars-T. Let's also try get some more feedback here before we move on with the new 3.x
Trying to reach out to yahyaalhamad - who was the last active maintainer. -> Assigning the issue to him, unfortunately the contact tab is not open.
I've reached out to the module maintains who have an open contact tab (Kars-T, focus13)
I created 💬 Offering to take over views_exclude_previews and develop new version Active now.
For reference, this is a ultimatively simple render-array as produced by XB:
{
"a548b48d-58a8-4077-aa04-da9405a6f418": {
"12345678-1234-1234-1234-123456789abc": {
"#type": "component_container",
"#component": {
"#type": "component",
"#component": "xb_test_sdc:props-no-slots",
"#props": {
"heading": "Hello from XB Component!",
"xb_uuid": "12345678-1234-1234-1234-123456789abc",
"xb_slot_ids": [],
"xb_is_preview": false
},
"#attached": {
"library": [
"core\/components.xb_test_sdc--props-no-slots"
]
},
"#cache": {
"contexts": [],
"tags": [
"config:experience_builder.component.sdc.xb_test_sdc.props-no-slots"
],
"max-age": -1
}
},
"#component_context": "Content Test Node (1), field field_component_tree",
"#component_uuid": "12345678-1234-1234-1234-123456789abc",
"#is_preview": false
}
}
Let's convert render arrays like this to custom elements as a first step.
I added comments to your MR at gitlab, not sure why they are not reflected here. please check https://git.drupalcode.org/project/custom_elements/-/merge_requests/125
the logic has been updated as proposed + test coverage is extended to verify this case works now. Furthermore existing tests were adapted as need to keep them passing by using the right production-url.
ci approves!
I made this work with the current config_ignore code, by re-using the event-subscriber class with dfiferent dependencies. That's not very nice, but works without requiring us to re-invent lots of config-ignore logic.
I created 📌 Allow working with different storage during export Active to hopefully find a better solution on the long-run, but for now this seems good enough. I tested the change manually and it works fine! During rebase the ignored changes are kept, during deployment ignored
Note: ran into an issue that return docs of getSlot() is off, so fixed it on the way.
created a quick test, with the help of claude code.
This is fixed via 📌 Error when installing module Active now!
It seems 📌 Support for Experience Builder Active is the next step and the way to go. This will integrate with 📌 Improve mapping render-arrays to custom elements Active but does not depend on it - on the contrary, figuring out the conversion strategy for XB-component render trees might help getting the solution for 📌 Improve mapping render-arrays to custom elements Active right. So let's do the conversion for XB-render trees first.
It seems would could do something like the following:
Allow conversion plugins to be registered. Identify the conversion plugin to use by #type or #theme key.
Examples of #type and #theme we'll need to implement
#type component --> logic for XB, see
📌
Support component and astro_island render arrays
Active
and
📌
Support for Experience Builder
Active
#theme custom_element --> straight conversion from custom element object
#theme block --> possibly special handling for blocks
Layouts use varying #theme keys based upon the layout, but have a #layout key. So we might want to support differentiating based upon that also.
Perhaps if we reworked the renderer so that all of the processing aspects were part of 'normalizing' and then the render array to HTML was 'serializing' we could support serializing to JSON and other formats for render arrays.
These sound like the 2 steps you're talking about in #8 @fago
yes! that would perfectly fit and allow other frontends to work with the serialized JSON response to implement alternating render strategies.
this is fixed and shipped with latest nuxtjs-drupal-ce module!
this seems to be fine. here an analysis by claude, which I agree with:
The dependency structure is correct:
- lupus_decoupled_ce_api.info.yml already declares drupal:path_alias as a dependency
- The service lupus_decoupled_ce_api.frontend_path_processor correctly depends on @path_alias.manager
- Transitive dependencies through the module system should make this workThe error might be occurring in specific edge cases or test scenarios where modules aren't being loaded in the correct order. The current dependency declaration
should be sufficient for normal operation
noted this is also fixing this
actually, this is a bug, since in reality d9 is already unsupported and untested, let's mark it as such.
Still, let's release next release as a new minor to better communicate that. I don't think it warrants to create a new major for that fix.
I updated the proposed resolution with a more detailed implementation plan as well as 📌 Prototype rendering JavaScript components with Nuxt Active .
After solving the nuxt-based component rendering I was a bit hesitant we are on the right track here, since the traditional Drupal theme's site-layout is just coming into our way. After planning out the big picture, I think it can work well if we achieve the following:
* Make the full-page preview of XB directly use the frontend via iframe
* Somehow avoid showing Drupal-theme site-layout in XB canvas - make the editor provide a blank canvas.
I forgot to mention: I moved the nuxt code for rendering component previews into its own re-usable package:
https://github.com/drunomics/nuxt-component-preview
So what makes this still hard to use the necessarsy proxy-setup in the backend + generating the right Nuxt-loader JS. I think both can be handled by a backend integration module nicely. We probably want to prototype the proxy code here though and verify it works well.
Also, the module will have to take care to initiate one nuxt instance on the page and use it for rendering all components. Technically having once nuxt instance by component would work, but it's a huge overhead that is likely to kill the browser with plenty of components, so that needs to be optimized.
fago → created an issue.
@fago in #25: AFAICT https://image.nuxt.com/get-started/providers is basically the same as https://nextjs.org/docs/app/api-reference/config/next-config-js/images#e...? Both are basically "generate a URL and let whichever server that points to parse its instructions from the URL".
yes, exactly. I think introducing this concept of pluggable providers/loaders makes sense, since generating all image-size variations is often taken quite some load/compuation-time and many variations on disk of image-heavy sites quickly become huge, would it make sense to add a pugin-type for that? (follow-up material).
Also not sure XB is the right home for this, this seems to be a generally very useful addition.
this should fix it
I also ran into this, with
🐛
Warning logged: Source path aut has to start with a slash.
Active
In this case it could be a module using the API wrong, what triggers the exception.
Here a quick example screenshot!
I suggested to start prototyping like that, because if that works, there's a clear path towards a new component source plugin, which is exactly how we started experimenting with code components. 🙂
Yes, this is just for easy prototyping atm!
@wim leers: Thanks a lot for the pointers and the summary! I'll definitely reach out for implementing the component-source plugin the right way, atm we are not reay for that yet, but I want to get there soon.
This prototype is coming along well, but did not explore matching vue component metadata (props/slots) to required format yet, so we can use that nicely register components. So there is some experimentation needed here before we can move on the component-source part side of things.
> Any chance you could share a screencast of it in action? 😇🙏
It's not fancy yet, but I'll see what I can do!
Adding some more details about the Nuxt/Vue component source plugin.
I have not started working on the component-source part yet, but what Nuxt is going to need is basically a "JavaScript component plugin with externally bundled JS". So this might be something we can develop generally and make usable with multiple JavaScript frameworks with little glue to invoke the components correctly, or shipping a base-class that easy easy to extend with the glue.