as I understand the problem is the configuration is not applied 1:1 as expected. yeah, we should allow setting some explicit '' prefix and simply do so when generating the element from config
thx, yes, we could introduce a new base class for tests, but nvm - main point is we have those tests now. Reviewed, fixed one comment and merged.
agreed. If a formatter returns
$configSubform = [
'settings' => [],
]
then the formatter shoudl be fixed to not return an empty form like this. The MR does this just fine, thus merged. thx!
Commented, please see remarks above.
Generally, change see to go into the right direction, but I dislike including the improvement of our ER-formatter here.
This should gets its own dedicated issue, so it appears properly in release notes. When doing so, it gets its own description why it is needed, and should come with some quick kernel test to demonstrate how it works correctly from a simple isolated API perspective. Maybe best, let's still have this issue, do it first, and then get back to finishing up here?
I figured the cache metadata was missing, added that and merged!
Thx! :-) merged!
commented, please re-check
alpha1 is there :-)
Thanks for bringing this home! When reviewing changes, I found an issue with it though. Created 🐛 Entity-CE-Display form incorrectly assumes every plugin has a settings-form key Active .
took a fist stab on this. it basically works already, but the ajax submission of the configuration form does not work yet. probably the entity-ce-display form has some problem there still.
thanks, works well, merged. thus, this point is still open, right?
- Empty plugin settings forms should not see a "settingsForm" edit icon
thx, good fix. Merged and setting back to needs-work for the main issue goals.
thx, merged!
ok, worked over it - as discussed with going with the semantics of "DISABLED" == not-existing as core does it usually also. Also extended test-coverage for making sure the fallback works correctly.
on this now
Some thoughts:
* We should check whether it would be enough to provide one CustomElementsFormController and a trait. less loptions, less confusion.
* Most logic seems to boil down to trait method
public function getCustomElementsContentResult(array $form): CustomElement {
return CustomElement::createFromRenderArray($form)
->setTag('lupus-form');
}
that we want to improve to render into a "drupal-form" component, which gets the data to render the
element, so it can gain some control over the form.
* By default, we should submit forms via the proxy, back to drupal via /ce-api prefix of the page. So when the route is ce-enabled, the form should be processed as ususal. Validation fails should work transparently that way. For that to work, we need to make sure the frontend correctly passes through POST requests. That should do away the need for all that custom-code for the user forms.
* For adding support for a form, a controllers needs to be provided which renders the form and creates the drupal-form CE via the helper like getCustomElementsContentResult() for the result. If form processing results in a redirect, lupus-ce-render should be able to handle that transparently.
@glynster: Thanks for your suggestion!
I'm not a big fan of adding something like nuxt-auth in the frontend, since it complicates the frontend stack, e.g. it becomes harder to support multiple frontends, upgrade nuxt majors, etc. But that said, nothing would speak against supportin this optionally. The advantage would be that it makes the frontend aware of permissions via oauth scopes, right?
Admin menu would definitely be a nice use-case, but in the end we only need a simple flag in page-api responses that tell us whether the users has backend-access/permission or not. Use-cases for permissions to check vary, so we could add a feature to allow permissions to be sent to the frontend as part of the page-api response. It smells a bit like a (simpler) rebuild of oauth-scopes though.
Maybe let's add a feature request and work on supporting that in a dedicated ticket?
As default, I think the cookie based access is fine, we just need to finish https://www.drupal.org/project/lupus_decoupled/issues/3336148 📌 Add support for forms Needs review to have user-login forms on the frontend working.
looks all good to me! however, the gitlab pipeline is failing with the changes. we need to make it accept it.
created https://www.drupal.org/project/custom_elements/issues/3446485 📌 Simplify entity-ce-display form and AJAX logic Active as follow-up
ok, I worked over the checkbox labelling and logic, please see
https://git.drupalcode.org/project/custom_elements/-/merge_requests/55/d...
I think we can merge this is as a first step.
> - Fix editing keys and pressing save bug
I took a quick look at this. This is not as simple as I hoped it is. Turns out there is some complex ajax-refresh logic going on, which is handled/implemented by the attached JS library "field_ui.js" ($form['#attached']['library'][] = 'field_ui/drupal.field_ui'). It seems this JS is atm necessary to handle updates of the region field when elements are moved into "hidden" and back to "content" region. This JS also somehow takes care of changing the submitted POST data to incude the "name" field.
let's also check whether we can drop this:
* \Drupal\custom_elements_ui\Plugin\Derivative\CustomElementsUiLocalTask::alterLocalTasks
* custom_elements_ui_local_tasks_alter()
seems unneeded to me.
checkbox: "Force rendering as custom element"
thx, slightly improved the text and merged it!
thx, changes work well, so merged.
But I think the sole button is not good UX. We need to explain the user why the button is there. Something like
"Layout builder is enabled for this view mode. Thus, the configured layout is rendered is getting rendered in a custom element. Configure the layout to customize how the element should be built."
Then show the button?
We won't change this anymore for 2.x - so moving to 3.x
it seems that a conclusion we came already on a year ago: there is a pre-existing ticket for this: #3323558: Make configurable Auto option build Custom element same as existing processors do →
I resolved conflicts with gitlab and merged results! Thanks!
Got it basically working. I've worked quite over the form. There are a few todos left, but I think it'S better to merge now, since we have something working, and do small todos in follow-ups. That helps to avoid merge issues with other MRs.
todos:
- Merge CustomElementsEntityDisplayFormBase into EntityCustomElementsDisplayEditForm
- Empty plugin settings forms should not see a "settingsForm" edit icon
- Fix editing keys and pressing save bug
so I guess we can simply drop all the CE-auto-generation now + have displayCE config be auto-generated and used on the fly by default. as BC, we could add a new flag in the displayCE config "forceAutoProcessing" which makes use of automatic processing for the whole entity, instead of applying the per-component configuration. Then everyone who wants BC can set this flag. We could even decide to expose it as some sort of setting in the UI later.
thx, this seems pretty good already. I dropped a few comments, please take a look!
thx. changes look good + tested it successfully.
WIP
Merged!
added some test-coverage to make sure config is passed through to the formatter correctly also. works now, MR is ready
quick draft
Took a stab on how it could look like in a fully BC way. What do you think about that?
https://git.drupalcode.org/project/custom_elements/-/merge_requests/49/d...
hmm, this is not so "optional" as I had thought of it.
Modifying all Processor classes can be done before updating the custom_elements module to version 3.x, without impact. Immediately after updating the module, all previously unmodified Processor classes will break.
That's not good, since it could result into severe upgrade headache. e.g. at lupus_decoupled we want to support custom_elements 2 and 3 in parallel until 3 is stable. Also that means custom code must be changed when updating, what makes it complicated. I think we should check and find a way to avoid this.
>The name should be optional, to keep BC for 2.x processors.
We really need this BC I think.
the way it works is that you simply put in the next upcoming version number in the change record, so it can be already be published. it should be published when the change is committed to the dev-version, so it's visibble for it. Thus, I added the version and published it.
Core field formatters store into entity-display component something like
component-of-field:
type: <formatter-id>
settings: <array-of-formatter-settings>
But core field formatters, they auto-generate some plugin configuration which looks the same. So their plugin configuration is something like:
configuration:
type: <formatter-id>
settings: <array-of-formatter-settings>
But in current 3.x branch our ce-field-formatters are having settings and configurations as same thing:
component-of-ce-field-formatter:
formatter: <formatter-id>
settings: <array-of-formatter-settings>
configuration:
formatter: <formatter-id>
settings: <array-of-formatter-settings>
custom-config: <value>
What is very similar. but our plugins can change configuration, not only settings.
Maybe we need to move it to clear separation:
component-of-a-formatter:
formatter: <formatter-id>
configuration:
settings: <array-of-formatter-settings>
custom-config: <value>
thx, all good. merged, thanks!
@roderik: Thanks, merged! Could you also add a change-record? See
https://www.drupal.org/list-changes/custom_elements →
We should document all breaking changes there
@roderik: Thanks, merged! Could you also add a change-record? See
https://www.drupal.org/list-changes/custom_elements →
We should document all breaking changes there.
finished step2 and made things work. Stumbled over quite some things, added lots of todos for them and created issues for 3.x.
Finally, I worked over tests and made them work again. Partially I added new display-ce configs to make things work, so we see them being taken into account.
Also, I had to work over the render pipeline such that it actually renders config first, and fallsback to processors second. Given that we should have the foundation for 3.x with this PR, but need to work through all of those smaller todos in follow-ups.
hmm. what is the use-case of disabling the default disable? I think we should simply disallow it.
thus changing issue title back. I think we should do the following here:
* Clarify how disable entity displays works and should work. Afaik there is no UI/feature to disable them in core? or is it the toggle of enabling/hiding the display tab of a view-mode config that enables/disables it? I think we should simply follow core here and to the same.
* Default display. Let's follow core and NOT allow disabling a default default display.
*
> indicate that in the UI as well IF settings are not taking effect since layout builder is taking over
Again, let's follow core. When the layout-builder is enabled for the view-mode, we should hide the regular form view mode and point the layout-builder settings, like core does.
patch looks mostly great! however, the url cache context for the new query parameter is missing. Added it and slightly refactored the PR to reduce code duplication.
I agree it makes sense to allow specialized controllers to take control. Patch is good and works as it should.
added a dedicated issue for auto-formatting: https://www.drupal.org/project/custom_elements/issues/3443794#comment-15... 📌 Add auto-formatter that uses regular processors Active
Created another branch and MR https://git.drupalcode.org/project/custom_elements/-/merge_requests/43 - which contains a first plugin for fields. For now, testing with a single 'timestamp' formatter.
Also, tried fixing the auto formatter, which needs work. It seems we need to improve our auto-processing to actually support a custom name to be given.
I started with trying to make the UI basically work. Somehow the changes are partically not saved atm. After quite some trial and error I figured that it is related to the ajax reload.
-->
After re-factoring, it's basically working now. Opened
📌
Make editing CE entity display basically work
Active
to work over the UI once we are fully done here and things work from a config.
I figured that entity-displays are still quite connect to core field formatters, what conceptually makes no sense in our case. We need to connect them to CE-field-formatters. For doing so, quite some changes were still needed to finish the introduction of CE-field-formatters. Thus, as a first step I worked over the code to make this work.
thx, merged test improvements code.
Commenting on the concept:
ad upgrade path: I'd worry about this last. We release a new major, so are free to do breaking changes.
Anyway, I think we should keep the module, but just drop all of the code and only ship with install-time config. That way, we can nicely proof what was done with custom code before, is now simply config. Once we have that we could also add an update function to install the module config, yes, then the update would be given also.
> Note that no solution has a guaranteed perfect upgrade path:
Not needed at all. There is hardly anyone using this sub-module besides us + we are releasing a new major, what already tells the message of: "Big changes, check your things"
> Fix RawCeFieldFormatter and turn it into a potential 'base class' for field specific formatters that need to set several values from one field (e.g. 'title' and 'href' from a link field). File followup issues as necessary.
We still have "Auto" what I think would make sense to do have reasonable default. In most cases auto + raw + any-core-field-formatter should do a great job already.
But yes, if it makes sense to have multiple options, multiple formatters can be added. We might need one for better entity-reference support.
That said, we don't have to keep 1:1 output of the thunder sub-module. It should be comparable, e.g. for links we should at least have those two attributes. If it has more, also good. But it should output the link as an array of data values. So if raw can do that, but has some additional data, that seems good enough.
Maybe generally, a good follow-up for "Raw" would be some optional config that allows one to define a limited list of properties to include.
@roderik
thanks, merged the tests-PR. This ticket got long and massive, let's add sub-tasks/issues for remaining/next steps?
I've left some non-blocking comments above, let's move any action items in sub-tasks/follow-up also, like one for the update path.
Setting to needs work for that, but when all follow-up/sub-tasks are added I think we can close this first one.
@roderik
Yes, that seems fine. We should not redirect any 4xx or 5xx errors and show them within Drupal.
Thus, I think the fix is good. But I'd suggest we also test-coverage for that to fix the behavior:
* 4xx / 5xx errors are shown within drupal
* Access frontend route like node/123 redirects to frontend
Not sure whether the seccond part is already covered.
thx, this seems pretty solid. I added a few small remarks
trying to summarize changes better in a short sentence, what about that?
You cannot add dev-dependencies in libraries in composer. well you can, but it would have no affect.
I think it might work to "suggest" it via composer suggest option + we can add it to our documentation.
Not sure what a test would do here? Maybe a unit test making sure the theme-registry service is not invoked when we call $imageStyle->flush($path);
First step:
* Ship with auto and raw formatters
Follow-ups we need:
* Add support for field formatters
* Add UI for configuring CE-formatters
* Fix support for entity-prepare view step
Converted the patch into a MR so the pipeline runs and slightly improved the comment.
It seems this caused some regressions:
After some more research I found out the change was introduced by 🐛 hook_image_style_flush doesn't get the $path passed to Drupal\image\Entity\ImageStyle::flush() method Fixed in Drupal 10.2 - a new optional $path parameter was added to image style flush hook. While doing so the flush() method was re-factory wrongly such that the theme registry + cache tags are cleared also when $path is given.
So both should be fixed. We've reproduced that addressing this and the related bug 🐛 With 10.2 image styles might get wrongly flushed Active solves the issues and brings those slow file upload requests down from ~25seconds to normal <2seconds.
Related change record: https://www.drupal.org/node/3373248 →