looks like @larowlan nailed it, unassigning myself
I don't want to block this, so I'm cool with #76. My concern was just Drupal doing something not aligned with de-facto standard by the major repo providers, so that when we're fully on GitLab we're not having this conversation again. Such as GitLab MR's automatically adding the co-authored-by and such. But we can run into the concern in the future if it even happens. Because right now this discussion is for maintainers who are forced to copy and paste a commit message.
Yeah, should be closed as a dupe and captured as an issue in ✨ Opt into static_cache for component config entities Active .
I don't know what I did that I swear I saw it in the UI beyond unit test. I'll follow up on #22
FYI post update hooks still run with the update kernel, I don't know why they're treated differently honestly. Dove into this a while back: https://mglaman.dev/blog/hookupdaten-or-hookpostupdatename
I know I touched the MR, but it's been changed quite a bit by @phenaproxima and looks great, and has addressed @alexpott feedback
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.
😱 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.
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
Makes sense to me. Let's tackle the common use case. We should also tackle video as well.
I'm not sure collecting this cacheable metadata will solve the problem. The `prop_field_definitions` is calculated inside of the component's config. This is calculated whenever the config entity is created or updated per
- \Drupal\canvas\Plugin\Canvas\ComponentSource\SingleDirectoryComponent::createConfigEntity
- \Drupal\canvas\Plugin\Canvas\ComponentSource\SingleDirectoryComponent::updateConfigEntity
- \Drupal\canvas\Plugin\Canvas\ComponentSource\JsComponent::createConfigEntity
- \Drupal\canvas\Plugin\Canvas\ComponentSource\JsComponent::updateConfigEntity
These invoke `\Drupal\canvas\Plugin\Canvas\ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase::getPropsForComponentPlugin` and update the `prop_field_definitions` settings which are passed as configuration to the component source plugin.
Canvas is regenerating SDC backed components in \Drupal\canvas\Plugin\ComponentPluginManager::setCachedDefinitions, but there's no way for us to push the cacheable metadata here. And the config changes happen after the cache is set, along with the fact `\Drupal\Core\Plugin\DefaultPluginManager::setCachedDefinitions` sets the cache tags based on the property for the class (okay, I guess we could dynamically alter that during processing.) But if we solved this for SDC's it wouldn't work for Code Components.
We just ran into this with testing out the Acquia DAM module support with image props. We had to re-save our Code Components so that a new version of the component was created that respected the new bundle. Technically all changes to the `prop_field_definitions` should trigger a new version and shouldn't be dynamically updated unless something re-saves/updates the component.
Now, that does make it pretty annoying if you add a new media type and it isn't usable, with no clear way to make it usable (need to trigger your component entity to be re-run with `updateConfigEntity` by its source.)
So I forgot that Canvas did not opt-in for static cache! We patched it this way. It's a bug, but bumping down to minor since it's not active in Canvas.
Tested it out. Looks great to me. And TIL about ajaxStop and other similar events that ajax.js emits.
@larowlan see changes to `tests/src/Kernel/ComponentInstanceFormTest.php`, it should add the necessary test coverage for a block disappearing.
@penyaskito pointed out that we have recursive ksort in the code for different areas, so I just copied that over.
I think this is another scenario: could be triggered by an image code component and someone creating another media type with an image media source.
Assigning to @wim leers per https://git.drupalcode.org/project/canvas/-/merge_requests/187#note_614299 for feedback before fixing the pipeline.
Tossing this back over to @wim. I've tried a few things and I've dove into the shape matching to feel much more comfortable, but I can't square up the matching here due to Canvas having custom schema attributes (x-allowed-origins, etc) that cannot be matched and if removed break a lot of assumptions.
This issue is lacking real-world steps to reproduce.
I thought this would be sufficient given it's how we reproduced the bug: "When using a code component with a prop of image or video you cannot publish a page using the default prop values. " It may not have been a bulleted list, but there is instruction.
The solution is a band-aid for the symptoms rather than fixing the root cause.
We had a bug blocking an upgrade to Canvas, so you're right, it's a bandaid. That probably should have blocked RTBC. But there wasn't a clear way forward as to what was clunky to un-clunk it per // @todo Make this far less clunky 🙈
The issue title + summary are misleading. It makes it sound like the ksort() is there for an arbitrary reason....That research+reasoning is missing from this issue.
This was quick fix to unblock upgrading to Canvas and resolving bugs. There isn't always time to perform Git and issue archeology. Sometimes we have to rely on others just providing that information.
Now that I've seen https://git.drupalcode.org/project/canvas/-/merge_requests/187#note_611531 I can look at the fix going into \Drupal\canvas\PropShape\PropShape::normalizePropSchema. The ksorts lived outside of \Drupal\canvas\PropShape\PropShape::normalize so the quick fix went there.
This is a dupe of 🐛 DefaultRelativeUrlPropSource does not sort properties keys for comparison RTBC I believe. Can you check that issue and the WIP patch to see if resolves?
I don't see how this would make a third option?
- describe the props/slots for developers
The description here should be just as usable for content authors. It's like a form description. the Canvas editor and reading the file should be the same.
RE ✨ [PP-1] Allow schema references in Single Directory Component prop schemas Postponed : I will look.
One question I have is: Will Canvas 1.0 be released before Drupal 11.3? If so, I think we should find a way to solve and deliver value now and then fast-follow for 11.3 if that issue lands. It also sounds like that issue will have breaking changes for Canvas anyway so a larger discussion needs to take place.
Don't forget we also use those props/slots descriptions as AI context.
Is that implying there is more work to be done? Or just calling out the importance of the issue?
FWIW durpal-mrn is setup to regex By|Authored-by|Co-authored-by to handle any changes.
https://github.com/mglaman/drupal-mrn/blob/main/api/src/CommitParser.php#L33
Personally I think we should use Authored-by and Co-authored-by and not By. We're copy and pasting the commit anyway.
Trying to sort out the best approach. But we need the field definition used in \Drupal\canvas\PropSource\StaticPropSource::getWidget to have a description set. Otherwise in \Drupal\canvas\Plugin\Canvas\ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase::buildComponentInstanceForm we can hack it in, assuming `$form[$sdc_prop_name]['widget'][0]` or something like that. But it'd be best if the widget used it properly from the field definition.
A prop may not be of type "object" unless it has a $ref defined.
This is being fixed in 🐛 References must not be required to guess props' JSON schema Active . Correct?
Required props must have examples and/or default
This seems to be the fix here, but per #8 it was descoped?
Canvas always requires schema, even for theme components.
Per #7 this seems to be the descoped item.
I'll try to sync w/ @lauri and verify the actionable item here is #2 from the issue summary and then adjust
Technically a bug, since we're fixing an edge case where some access-based cacheable metadata could be lost
I know core has translation tests, but here's an example from work we did in Commerce that may be more succinct to help test this: https://git.drupalcode.org/project/commerce/-/blob/3.x/modules/product/t...
Looks like this can happen if max loops are reached, also:
$this->eventDispatcher->dispatch($event, AgentStartedExecutionEvent::EVENT_NAME);
$this->looped++;
if ($this->looped > $this->aiAgent->get('max_loops')) {
return PluginInterfacesAiAgentInterface::JOB_NOT_SOLVABLE;
}
the design doesn’t have the menu tabs.
I think that's because it was intended not to be part of /admin/content and friends. That would remain for structured content (nodes, media.) So maybe we keep this as is an open a follow up to clarify/decide? 1) decide tab order, 2) move out of /admin/content
We could try to guess the tab order. But I have a feeling it'd be buggy or random with Views in the mix.
I have a weight set in my link definition. Maybe removing it will fix it? I'm not sure as the ordering between tasks and menu links is not associated at all
Linking to ✨ [PP-1] Add a local action to the pages listing to create a new page Active as it adds the collection route as well.
Got in some tests and fixed issue reported by @tedbow
We had this in Commerce and added these helper methods to our entities: https://git.drupalcode.org/project/commerce/-/blob/3.x/src/Entity/Commer...
Seems like we need similar helpers to make sure referenced entities are translated.
1. Someone may not have access due to hook_entity_access or hook_jsonapi_entity_filter_access which replaces the entity with an inaccessible object
2. Someone may implement hook_field_access and some fields aren't in the response for anonymous users
I'm am thinking having requests shown as unauthenticated by default would be best and give more realistic examples. I think it could start as a checkbox as "Use authentication". Words are hard, maybe borrow whatever the OpenAPI doc viewers use. Eventually it'd be cool to allow picking a Consumer for Simple OAuth to demo the scopes.
It has to be something related to the dataDependencies in a code component on the site, but I'm not sure how to track that down.
I also think it proves there's something broken in this libraries.yml logic
canvasData.v0:
dependencies:
- canvas/canvasData.v0.baseUrl
- canvas/canvasData.v0.branding
- canvas/canvasData.v0.breadcrumbs
- canvas/canvasData.v0.pageTitle
- canvas/canvasData.v0.jsonapiSettings
drupalSettings:
canvasData:
v0: []
canvasData.v0.baseUrl:
dependencies:
- core/drupalSettings
drupalSettings:
canvasData:
v0:
baseUrl: null
canvasData.v0 depending on canvasData.v0.baseUrl does not cause canvasData.v0.baseUrl to be null but canvasData.v0 is null (or canvasData is actually empty)
Fix isn't great. I also couldn't reproduce locally but can on a hosted site.
Seems like this is really about adding MenuTreeResource. I don't like how this is implemented as piggy-backing the existing resource, decoding the response, and transforming it. It could easily be done as another service.
We could add `MenuTreeResource` and a supported route. That resource uses a tree storage. I don't think it should be exposed automatically but it can be via a feature flag ($settings or config)
It's not Gin's fault. See
That's because the theme switches from gin to xb_stark.
It's the Canvas theme negotiator's fault.