🇦🇺Australia @larowlan

I'm being super paranoid here, but I think we should add a requirement to the system.files route (via a regex) that prohibits module and theme as valid schemes for the /system/files/{scheme} path

Yes, we'd need a hook_file_download to return a header - but I'd hate for a dud hook_file_download in contrib to lead to a vector where people can download the source code of modules/themes - yes many themes/modules come straight from Drupal.org but most projects have custom code.

I think its a simple addition to give us some additional hardening.

larowlan → changed the visibility of the branch 3470422-be-nice-for-sdc-developers-larowlan to hidden.

larowlan → changed the visibility of the branch 3470422-be-nice-for-sdc-developers to hidden.

Added child issues and updated the issue summary to reference them

larowlan → created an issue.

larowlan → created an issue.

larowlan → created an issue.

larowlan → created an issue.

larowlan → created an issue.

I postponed #3556327: [PP-1] Don't save component config entities during discovery in an update kernel → on this because the new tests here will give us coverage for it.

Postponing on 🐛 Text formatted with CKEditor within Canvas gets double escaped when output Needs work because we'll get test coverage for free from that, the number of versions for the SDC component we update will now be 3 instead of 2.

Opened #3556327: Don't save component config entities during discovery in an update kernel → for #18

A comment from @mglaman on 🐛 Canvas' hook_storage_prop_shape_alter() must allow passing cache tags — to know when to re-evaluate Active made me realise the 2 vs 3 versions in the update test is actually a symptom of a bigger issue - so I opened #3556327: Don't save component config entities during discovery in an update kernel →

larowlan → created an issue.

😱 I didn't even think about the fact UpdateKernel uses NullBackend so the plugin managers are always calling setCachedDefinitions and causing component churn, unless the update hooks mark config as syncing.

We should consider opening a new issue to prevent this, e.g. by checking if we're in an update kernel in the component manager and block manager before we blindly save new entities - if this happens during database updates we're breaking the rules around what you can do in an update kernel vs what should wait for the post update hooks. Namely, using the Entity APIs is not supported in an update kernel until post update hooks.

Turns out hook_media_type_insert is too early, because at that point the source field doesn't yet exist. So used hook_field_config_insert and the equivalent update hook instead. Works just fine!

This doesn't cover any contrib-based source plugin that extends from \Drupal\canvas\Plugin\Canvas\ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase

But I think we can live with that.

larowlan → changed the visibility of the branch 3547579-hook_storage_prop_shape_alter-allow-specifying-when-to-reevaluate to hidden.

Working MR up https://git.drupalcode.org/project/canvas/-/merge_requests/298

larowlan → created an issue.

Yes we had the same issue with automatic regeneration in 🐛 Text formatted with CKEditor within Canvas gets double escaped when output Needs work yesterday - i.e. the fact SDC component config entities can be resaved on a cold cache during discovery meant that update hook changes were applied during discovery rather than during the post update hook.

The same applies for code components.

I think yeah we need a hook_media_type_insert/hook_media_type_update here to flag that we need to regenerate code component config entities - we can query for JavascriptComponents with a prop of type `$ref: 'json-schema-definitions://canvas.module/image'` and resave them

I'll have a crack at that

Thanks

Is there any way we can write test coverage for this?

We have existing examples in core that create modules on the fly during the test - see \Drupal\Tests\system\Functional\Theme\MaintenanceThemeUpdateRegistryTest::prepareEnvironment we could write a test module to $this->siteDirectory . '/modules/' that has a block plugin, and then delete it and assert the expected behaviour

ah updateExistingComponentValues is used in a test component ConceptProver - is that module/test coverage still needed? the module name is canvas_test_extension_legacy

I think we can remove updateExistingComponentValues if it's not used, no sense in having API in a stable and then needing to support it forever.

For _updateExistingComponentValuesForLinking - I agree - defer that to when we have more tests

I'll update the MR to drop updateExistingComponentValues

Attempting to update the issue summary to reflect the comments that track how this progressed from 3 items to just 1 and the current state of the MR.

Code changes look good to me - I think this is RTBC

Current code looks good to me and has product manager signoff in #6

Re #19 I think there is merit in a UI schema but that is out of scope for here and would need to be something we did in core as a feature request - can you open an issue for that?

Left two comments that I think can likely be followups given this is RTBC and critical

Couple of test fails here that look related to assumptions about the previous behaviour

This is a great improvement 👏

This caused a fatal error on 11.x for Canvas which has a semi_coupled theme engine.

Is there a BC layer that should work until any existing engine has a chance to update?

Error 500: Service "semi_coupled" not found: the container inside "Drupal\Core\Theme\ThemeManager" is a smaller service locator that only knows about the "twig" service.

Left a review on MR 196 (Which I think is the right branch to review based on recent comments)

Marking as needs work for XSS safety.

Left some questions, but this looks nice

We need a change record here and we aren't actually emitting a deprecation error yet for those with components in their default config.

Nice one!

I confirmed all the edited css files are just generated with this script

git diff origin/11.x --name-only|grep css|while read r;do J=${r/\.css/.pcss.css};if [ ! -f "$J" ]; then echo $J;fi;done

Which printed nothing 👌

Committed to 11.x, backported to 11.3

Updated the solution to make use of the ajaxStop event Drupal fires and was able to remove the need for timeouts
Added a playwright test that replicates @bnjmnm's excellent debugging and changes to the test module

If you want to manually test this, revert commit 8cc00e69023d8b49f5b8efeb1c46a74dde914539

poker10 → credited larowlan → .

This was resolved in 📌 [PP-2] Accept Dynamic Props in ApiTemplateLayoutController for content templates Postponed which removed that function in favour of \Drupal\canvas\Controller\ApiLayoutController::getEntityWithComponentInstance which uses \Drupal\canvas\Plugin\Field\FieldType\ComponentTreeItemList::getComponentTreeItemByUuid

📌 Replace the postPreview action with atomic equivalents Active most likely won't go in before stable

larowlan → changed the visibility of the branch 3539693-inputbehaviourscommon-mutates-the to hidden.

Rebased as an MR against Canvas, refined reasoning and title

larowlan → changed the visibility of the branch 3539693-inputbehaviourscommon-overwrites-the to hidden.

Actually it's simpler than that

BlockComponent::validateComponentInput references getComponentInstanceFormViolations already

The issue is EntityAutocomplete sets the input value to NULL when the user entered an invalid value.

So when we validate, what the user entered is no longer there.

The solution is to restore whatever the user entered into the component inputs if there's a validation error.

That way later when we call validate in a publish context (outside a form context) the original entered value is still validated.

Was able to easily adapt our test to simulate this and get a fail/pass.

Ah \Drupal\canvas\Controller\ApiAutoSaveController::post checks $this->autoSaveManager->getEntityFormViolations but not getComponentInstanceFormViolations!

We have this in \Drupal\canvas\Controller\ApiAutoSaveController::post

        $violations = $entity->validate();
        $form_violations = $this->autoSaveManager->getEntityFormViolations($entity);
        foreach ($form_violations as $form_violation) {
          // Add any form violations at this point.
          // @todo Remove this in https://drupal.org/i/3505018
          $violations->add($form_violation);
        }

And that gets written to in \Drupal\canvas\Plugin\Canvas\ComponentSource\BlockComponent

// Store block form validation errors so they can be used later during
          // component validation.
          $this->autoSaveManager->saveComponentInstanceFormViolations($this->formId, $violations_list);

But I do see we skip required fields - so perhaps that's the issue - will debug specifically with Webform.

Picking this up

thanks @rikki_iki, shows I didn't read the comments here, just went straight to the MR from the issue summary. Bring on gitlab issues!

Left some comments. Really excited about this

Left some comments on the MR.

Also tagged as needing a change record - let's promote this!

Nice work folks

Fix looks straight forward - let's get a test here - the steps to reproduce seem pretty simple

Thanks!

Left some comments on the MR
Fine to self RTBC after updating/replying

Published the change record

Committed to 11.x and backported to 11.3.x
Published the change record
thanks!

📌 Passing a $options array to constraint constructors is deprecated, use named arguments instead Postponed is in

Committed to 11.x and backported to 11.3.x
Thanks!

Do you have a sample of what this yml will look like? Are you thinking something in the info.yml that maps blocks to regions for the theme?

Also, is the next step here to put this into a validation constraint and port EntityChangedConstraint into an event listener that extends the base discovery one?

This is green now

I have some thoughts on the naming.

We're calling the two revisions 'local' and 'remote' but I'm wondering if we should stick with the naming that git uses, namely 'ours' and 'theirs' as I think that is clearer which is which.

Thoughts?

Patch from the other issue up as a MR and updated to current core standards etc

Working on the rebase as an MR

Just one question on the MR, fine to self RTBC

Committed to 11.x and backported to 11.3.x

Published changed record

Thanks all

Left one question on the MR but otherwise this looks really nice
Reviewed the change record and it looks good too

I think what we want here is a `hook_modules_installed` in field_layout module that does what layout_builder.install is doing if the module being installed is layout_builder. I.e flip the responsibility from LB to field layout

Looks good to me

The approach here looks good to me
For those interested here's the composer docs on branch-alias

Looks like there's a conflict on the composer.lock file now

In the meantime, since we're already doing our own import map anyway, we should allow this alteration. Once there is a core API, we can revisit this.

Seems reasonable, but APIs are forever, so whatever we do we need to consider we might need to back it out when there's a core API.

Are there tags that could be invalidated instead?

greggles → credited larowlan → .

mcdruid → credited larowlan → .

So, with the visibility shared in #45, does that means Mechanism for page wide application would need some more discussion and/or work? The API is already very valuable without this mechanism, because local applcitaion is the most common use case. So we are OK to remove core/lib/Drupal/Core/EventSubscriber/HtmlStylesResponseFilter.php from the MR.

In slack during discussion with @grimreaper I posted

If the bubbling to the body stuff isn't the 80% use case my advice would be to remove it. We should focus on MVP for core

It sounds like this bubbling isn't the main use case so I think we're in agreement there - thanks!

Can we get an issue summary update here - what's the use case for this - why do we need it etc?
@grimreaper asked me to review it but I'm missing the context.

It looks like you can put #styles on any element and they bubble up with attachments and then get replaced.

I'm not super keen on the HTML rewriting - can we do that with placeholders like we do for scripts/css in \template_preprocess_html?

Left some questions on the MR

I would like to opt in link_attributes, entity_hierarchy, aggregator, forum, media_file_delete to start.

But really, I've got 70 odd modules and would be happy for any of them, so feel free to pick whatever is best for you in terms of size/complexity.

Happy to be a guinea pig

Thanks. fixed, tagging 8.x-2.4

larowlan → made their first commit to this issue’s fork.

larowlan → changed the visibility of the branch 3550334-wim-shall-not-merge-while-multitasking to hidden.

larowlan → changed the visibility of the branch 3550334-text-formated-with to hidden.

Just one comment on the MR, fine to self RTBC after that I think

Thanks! Looks great so far, but needs an upgrade path (post_update function?) for existing Component config entities (anywhere else?) unless those are automatically regenerated with this new expression on cache clear.

They will autogenerate but I _think_ that will create a new component version, so any content pointing at the old version will likely get double escaped. We might want to do an update hook to update the version of any content pointing to that version.

Credited @shitalb who also asked about this when the issue was private and was added there.

Moved the private MR to this issue

larowlan → made their first commit to this issue’s fork.

larowlan → created an issue.

larowlan → made their first commit to this issue’s fork.

@heyyo I've added some changes and additional tests to a new MR here.

The test shows we can recover from:

* Missing component
* New props (including new required props)
* Old props still present

Can you give it a test?

Thanks in advance!

So far so good w.r.t not needing an upgrade path.

larowlan → changed the visibility of the branch 3532514-robust-generated-props-form to hidden.

Pushed two extra commits to a new branch in 3470422-be-nice-for-sdc-developers-larowlan

I'm planning to use that approach in 🐛 Deleting or renaming SDC prop will break xb pages using old version of the component Active

Also includes tests (that will fail with the current code in the branch for this issue) for when the missing component has slots and there are child components in those slots.

To resolve that I was forced to go with the approach in 3470422-be-nice-for-sdc-developers-larowlan, which is why I think we need that here. It is a best-effort attempt but without the update path we'll need for 🐛 Deleting or renaming SDC prop will break xb pages using old version of the component Active

Rather than rely on locale module's parsing of JS, we could also attempt .po extraction with a Vite plugin - the moduleParsed hook in rollup (which Vite uses for building for production) should afford us access to the AST where we could find calls to Drupal.t() in a cleaner fashion that the regex used by _locale_parse_js_file which has had issues in the past