🇪🇸Spain @penyaskito

Disclaimer: I usually work with 11.x HEAD, so was surprised I didn't see this before.

The problem is that if installing several modules, after experience_builder is installed no rebuild is happening, so it's not triggering the discovery of sdc components/blocks and its related config creation, and xb_test_config_node_article depends on that.

I thought we might need to flush caches on hook_install, but we are actually doing that already.
That's why the issue only happens in (some) kernel tests.

So we have two options:

1) Include the config in xb_test_config_node_article (meh 😔)
2) Document and workaround it in kernel tests (☺️)

e.g Never use

$this->container->get('module_installer')->install(['experience_builder', 'xb_test_config_node_article', 'sdc_test']);

but instead use

$this->container->get('module_installer')->install(['experience_builder', 'sdc_test']);
$this->container->get('module_installer')->install(['xb_test_config_node_article']);

#4 🐛 Block plugins need to be able to update their settings in multiple different storage implementations Active I haven't looked at Dashboard module internals, but pretty sure that also needs the same treatment.

Confirm, we would need to do the same, and we are not doing anything for this scenario (in this case I'd expect layout builder implementation to handle this for everyone using a SectionStorage plugin).

#0 🐛 Block plugins need to be able to update their settings in multiple different storage implementations Active Conversely, updates are not the only place that this logic needs to run, it also needs to run on presave of all these different config entity types to support shipped config too.

IMHO this will be complex enough that it could be left for a follow-up, and AFAIK we have no precedent of this. Looks like a related-but-not-the-same separate beast.

E.g. if node module implements a config schema change on node_type, the shipped node types config of contrib module X are not updated on installation of module X.

#8 🐛 Block plugins need to be able to update their settings in multiple different storage implementations Active @longwave mentioned in slack the possibility of generalising this to all plugins that have settings.

If we include shipped config on the scope, and per my comment above, wouldn't this be a general problem for any config that has an update to its structure/config schema, and not only plugins?

And I'd expect we would need some kind of tracking of the last hook_update_N/post_update where this shipped config was generated with, which might mean a huge change with no BC layer.

Fix adding deps_plugin to IS.

@bnjmnm looked at this, @larowlan saw latest changes by @bnjmnm, both have seen my changes and I just did a clean rebase, so RTBC

For 🐛 ContentCreatorVisibleXbConfigEntityAccessControlHandler's `view` access must refuse access to disabled config entities Active I ended up hitting the openapi schema bug described by Ted @ #6 🐛 XbConfigEntityHttpApiTest Active .1. So that's covered there.

You can export your config (see https://www.drupal.org/docs/administering-a-drupal-site/configuration-ma... → ).

The dashboard will be some config with the prefix dashboard.dashboard.. Take into account that it might depend on other configuration, that you will need to export to and import all together into your other site.

Deleted comments field from articles, have recent_comments view enabled, applied 📌 BlockComponent::checkRequirements() should disallow any block plugins whose definition declares it requires certain contexts Active , I still see:

Warning: Undefined array key "base" in Drupal\views\Plugin\views\query\QueryPluginBase->getEntityTableInfo() (line 302 of core/modules/views/src/Plugin/views/query/QueryPluginBase.php).

and components don't load with an error on xb/api/config/component:

"A valid cache entry key is required. Use getAll() to get all table data."

larowlan → credited penyaskito → .

The only failing test is because of 📌 Cypress test publish-validation is flaky Active

penyaskito → created an issue.

> Update all of our API responses (both GET individual and GET list) to provide the entity operations' link relationship for those operations that are permitted

There's no individual GET for ApiContentControllers. I don't think this should be also on auto-saved data, so this should be good now?

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

I confirm that with 🐛 SDCs with optional images without examples cannot be placed Active MR the patch request works and the content is saved as expected.

But something confusing for users happen. When I edit the heading after adding the component, the image is still there. When I refresh the editor, the image isn't there (because it was optional, makes sense).

Should the default optional image disappear once we edit any prop? That would be confusing for the editor as some element is disappearing and might be hard for them to find it again to set an image.

Should we have the optional image placeholder show after saving? That would be confusing for the editor as the preview won't match the actual rendering of the page.

Even if this is not a BC break technically, it would be in practice, as the components I had to change in core show.

So probably a change record won't hurt, and we might not want to cherry-pick to 11.1.

@mglaman That includes the config-related permissions, which are all-or-nothing so are just flags. But we might want to still postpone on 📌 Update `ApiContentControllers::list()` to expose available content entity operations in `meta` Active for the content related ones.

Type was never defined in schema for slots, but some components were using it.

I opted for removing it, as slots should be any html fragment, and we aren't validating them anyway.

Follow-up: 🐛 Disable additionalProperties in slots, props in SDC json schema Active

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

#13 See playwright toMatchAriaSnapshot for inspiration? https://playwright.dev/docs/release-notes#version-149

I'd like to see JsComponentTest to look more like BlockComponentTest and SingleDirectoryComponentTest. Not having a setup() will make easier to test the discovery too, aside of providing consistency in the 3 tests.

We could use some dataProviders here and there and avoid some helper methods in JsComponentTest

Tested this manually and works for me.

Reviewed and run the tests locally and all good. 🚢

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

Isn't this a duplicate of 🐛 Disable additionalProperties in slots, props in SDC json schema Active from core?

Added 🐛 Disable additionalProperties in slots, props in SDC json schema Active as related.

The patternProperties regexp means that properties that match that regexp will require those validation rules. But not that properties MUST match that regexp.

Added 📌 Twig disallows dashes in variable names, so SDC should disallow it in prop names Active as related.

allOf didn't work, as this is an object not an array, so reverted that.

Added a test with invalid slot name, and apparently additionalProperties is ignored, becase this test fails to trigger the expected exception.

So we can do this for slots, because we have our own slotDefinition, but not for props, that are using http://json-schema.org/draft-04/schema#: see https://github.com/orgs/json-schema-org/discussions/502#discussioncommen....

Maybe we can use allOf instead of $ref here.

This doesn't looks like the right solution but avoids all the preprocess warnings:

diff --git a/src/ClientSideRepresentation.php b/src/ClientSideRepresentation.php
index 4637364..2f0d636 100644
--- a/src/ClientSideRepresentation.php
+++ b/src/ClientSideRepresentation.php
@@ -49,6 +49,11 @@ final class ClientSideRepresentation implements RefinableCacheableDependencyInte
     }

     $build = $this->preview;
+    // If it's a block, the preprocess won't have the necessary info and will
+    // throw lots of warnings.
+    if (isset($build['#theme']) && $build['#theme'] === 'block') {
+      $build['#configuration'] += $build['#configuration']['default_settings'];
+    }
     $default_markup = $renderer->renderInIsolation($build);
     $assets = AttachedAssets::createFromRenderArray($build);
     $import_map = ImportMapResponseAttachmentsProcessor::buildHtmlTagForAttachedImportMaps(BubbleableMetadata::createFromRenderArray($build)) ?? [];

This has new tests, removes xb_visible_when_disabled, and behaves the same way as HEAD.
The only issue is that JavaScriptComponent is singled-out:

    $require_status_true = match ($xb_config_entity_type->getHandlerClass('access')) {
      ContentCreatorVisibleXbConfigEntityAccessControlHandler::class => FALSE,
      XbConfigEntityAccessControlHandler::class => ($xb_config_entity_type->id() !== JavaScriptComponent::ENTITY_TYPE_ID),
      default => throw new \LogicException(),
    };

This means that Pattern probably should have had the xb_visible_when_disabled flag too.
NR for confirmation.

LGTM!

Not sure it's a strict requirement, but we might want to track 🐛 Disable additionalProperties in slots, props in SDC json schema Active

wim leers → credited penyaskito → .

penyaskito → created an issue.

@jessebaker Will this still be true with 📌 Selecting multiple components Active ?

wim leers → credited penyaskito → .

wim leers → credited penyaskito → .

This was not catched on the tests. The select "clears" visually, but the data is stored and posted correctly, and after publishing and refreshing the page you can see the values you selected.

The cause is the options are casted as ints in parseValue at function-utils.js. when the values on the select are strings. It happens too in field_xbt_daterange_datelist.
Not sure how to fix this visually AND on the stored data, which requires ints. But hope this helps.
We would need some way to ensure in Cypress that the value selected is actually visible too. We are actually forcing the hidden select.

    // Radix renders these as a hidden element with a button to trigger, so
    // we have to use force.
    cy.get(`@${key}`).select(String(value), { force: true });

but we need to check the button label is changed too to the right value I guess?

Even if the test is passing, when I select a value the select box "clears". We should show the selected option.
I'm guessing this is caused by the change in 📌 Add e2e test and confirm support for date range date list widget Active , as @isholgueras reported this worked for him.

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

Should be green now, great catch @tedbow!

penyaskito → created an issue.

(We can check in tugboat for this MR: https://mr59-be4pskurppwvqfqo7pjfutudyzyllyze.tugboatqa.com/en/admin/str..., login as admin/admin)

I think it makes sense to just wrap in a $dashboard->access('view') check.

It could look weird if you don't have access to a given dashboard that won't be linked, but I guess we can live with that.

Just found that we don't have any test for the listing, so might be a good opportunity to add one.

You sure? there's an extra 2

> 3.2222? Then write it again: 3.22222

penyaskito → created an issue.

Fixed everything from Wim review, added change record draft.

#20 suggestion is great, I'll fix that.

Thanks! Added 🐛 Property constraints validation adding errors to the field element instead of just the property Active for exploring other widgets.

penyaskito → created an issue.

Fixed feedback, thanks!

OK, I'll stop messing around. It's already fixed, just we can't resolve threads.

NW per https://git.drupalcode.org/project/drupal/-/merge_requests/11220#note_49.... We should accept that suggestion, then IMHO we should be good to go.

This happened in #2470952: Path deletion should be removed in path module after content translation removal →

Which you can workaround when these issues land, if I'm not wrong:

https://www.drupal.org/node/3496786 →
https://www.drupal.org/node/3497308 →
https://www.drupal.org/node/3493962 →

We still need to cover #6 tho.

> I don't think that's a good idea, that could be tens or hundreds of thousands of revisions.

Also, I might have some contrib/custom module modifying that message, so we shouldn't touch existing messages at all.

Reviewed the code, checked the test coverage, this looks safe to me.

Added an end-to-end test (as a kernel test). This is ready from my POV.
The test failure is happening in HEAD too.

Per #28, #29, close won't fix. At this point events don't provide any benefits than hooks don't cover already.
Thanks everyone who worked here.

Thanks!

balintbrews → credited penyaskito → .

Easy enough to fix in the browser and test on tugboat :-)

This is still needs a proper resolution.

a) Add a new restricted permission for language.negotiation_url route, flagged with "restrict access".
b) Flag "administer languages" as "restrict access".
c) Change nothing, but document this in language.permissions.yml?
d) Do nothing.

I tempt to think that c is the right one.

Agree this would be testing core, which is not our purpose.

penyaskito → created an issue.

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

function experience_builder_config_schema_info_alter(array &$definitions): void {
  // This allows these blocks to be placed for now, but really the schema
  // should actually be FullyValidatable.
  // @todo Fix this in core or solve this another way in https://www.drupal.org/project/experience_builder/issues/3484666
  $types = ['block.settings.system_menu_block:*', 'block.settings.views_block:*'];
  foreach ($types as $type) {
    if (isset($definitions[$type])) {
      $definitions[$type]['constraints']['FullyValidatable'] = NULL;
    }
  }
}

📌 Define `props` in the context of Block components Active was fixed, but we never did anything with this @todo. Is it time to remove this alter?

Added the related issue where this will be fixed in core.
The comment from @ultrabob is the right workaround in the meantime.

Someone took the time to write more concrete steps for the workaround here: https://www.drupal.org/project/social/issues/3410322#comment-15710653 🐛 Impossible to translate "ago" on the list of private messages Needs work

This is the wrong approach.

You need to translate your view/form mode formatter/widget settings with config translation, not translating a dynamic string with interface translation.

See 🐛 Entity view/form mode formatter/widget settings have no translation UI Needs review for the proper solution. Marking this as duplicated.

As a workaround you can mangle with your yml config files and import them.

I've not been able to reproduce this.

• dashboard 2.x
• layout library 8.x-1.0-beta5 (last release, current HEAD)

do we need tests for this one?

For testing:

Log in having some dashboards.

1. Press Alt + K. Shows the coffee bar.
2. Write a dashboard id. Show show it and link to the dashboard.
3. Write a dashboard label. Show show it and link to the dashboard.
4. Write ":dashboard". Should show only the dashboards.
5. Verify doesn't show any dashboard you don't have permissions to.

@mondrake ghost of drupal past explained another way to reproduce the bug, more realistic, which is not influenced by what connection holds. Added another unit test for covering that.

So even if what you suggest would work in that case (and I hope we can start doing that all over core), wouldn't work on this one.