🇪🇸Spain @penyaskito

Tests are green. Phpstan issues are not introduced here.

Apparently something went wrong during the rebase.
Cleanest+easiest way forward might be opening a new MR from a new branch up-to-date with 0.x.

Check https://git.drupalcode.org/project/experience_builder/-/merge_requests/6... for the starting point that @longwave did.

After a new review this makes lots of sense. I misread the MR.

However, we can just do this in \Drupal\dashboard\Plugin\SectionStorage\DashboardSectionStorage::getRedirectUrl

Something like this should work.

  public function getRedirectUrl() {
    return Url::fromRoute('entity.dashboard.canonical', ['dashboard' => $this->getStorageId()]);
  }

Other changes should not be necessary, aside of the added test.

Found while looking at 🐛 Drupal CMS Needs review .
Builds on top of it.

penyaskito → created an issue.

In this page, using Spanish and no installing on the root, drupalSettings.path contains

baseUrl	'/'
pathPrefix	'es/'
currentPath	'admin/modules/browse/drupalorg_jsonapi'
currentPathIsAdmin	true
isFront	false
currentLanguage	'es'

I'm pretty sure we want

export const BASE_URL = `${window.location.protocol}//${window.location.host}${drupalSettings.path.baseUrl}`;

because that's the BASE_URL. The prefix might or not be there.

Usually in core you can use Drupal.url('/admin/content'), and that resolves the proper base url and prefix to use.
I think we should replace most if not all usage of BASE_URL in the svelte app with that.

For the issue at hand should be enough to ensure FULL_MODULE_PATH doesn't have the prefix.

I can see that with CMS 1.1.x + Dashboard 2.x, the icon is there but there is a square on top of it.
Which doesn't happen with core 11.x, but 11.2.0 won't be released until June.

We might need to revisit this again 😭

penyaskito → created an issue. See original summary → .

Now that we have hook_navigation_defaults hook in core is much easier to implement. This will only work on >=11.2 though.

Agree that makes sense to place it in custom. But we need to ensure the folder exists, or create it in the script.

Back to RTBC then 🎉

Needs review for https://git.drupalcode.org/project/experience_builder/-/merge_requests/7....

I'm pretty sure that's a good fix, but want to be sure that we aren't just hiding another issue, as might be related to 🐛 ComponentTreeHydrated prevents serialization Active .
Assigning to larowlan for that.

The trace of the test failure was:

/var/www/html/web/core/lib/Drupal/Core/Entity/EntityStorageBase.php:262
/var/www/html/web/core/lib/Drupal/Core/Entity/Plugin/DataType/EntityReference.php:74
/var/www/html/web/core/lib/Drupal/Core/TypedData/DataReferenceBase.php:37
/var/www/html/web/core/modules/serialization/src/Normalizer/EntityReferenceFieldItemNormalizer.php:44
/var/www/html/vendor/symfony/serializer/Serializer.php:152
/var/www/html/web/core/modules/serialization/src/Normalizer/ListNormalizer.php:24
/var/www/html/vendor/symfony/serializer/Serializer.php:152
/var/www/html/web/core/modules/serialization/src/Normalizer/ContentEntityNormalizer.php:25
/var/www/html/vendor/symfony/serializer/Serializer.php:152
/var/www/html/web/modules/contrib/experience_builder/tests/src/Kernel/DataType/ComponentTreeHydratedTest.php:957

The normalization is trying to load the anonymous user from the db, and the users table doesn't exist.

NW per the test. Also not sure why changing the API is necessary here.

Fix + test

Encountered while pairing on debugging #3390712-52: Add component variants to SDC → with @larowlan. Please credit him.

Paired 1.5h with @larowlan and we got with the problem, fixed it and we added a test proving that validation is happening.

The problem came from copy-pasting slots json schema definition. The problem is originally there and we all overlooked it :-(
Created 🐛 SDC slots not being validated against json config schema Active for fixing this for slots.

Thanks a lot Lee! (please credit him)

penyaskito → created an issue.

Granting credit to @larowlan for previous reviews now that I can.
NR again, maybe last time? 😁

This was fixed in 🐛 Dashboard icon missing from Navigation Active , just not yet released.
Does it happen to you if you try the -dev release?

MR on top of 📌 Add access control for "code components" and "asset libraries", special case: instantiated code components must be accessible to *all* Active .
The actual thing to review here is since 2696a5.

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

I've debugged the JsonSchema/Validator and still can't find why invalid data is accepted.
This might need fresh eyes.

@pdureau You convinced me, no changes in ::generateComponentTemplate() and moved that logic to ::preRenderComponent as suggested. Thanks for taking the time to explain the reasoning behind doing that.

Only pending thing is some test coverage @e0ipso requested, and verify if json schema validation is being triggered properly, as I don't think we need any extra validation that that?

bot crossposted?

phpstan errors are unrelated and fixed in 📌 Update PHPCS config to match Drupal 11 + phpstan issues Needs review .

Found a couple of other routing changes required.
Also fixed docs referencing these.

This should be good, @megasharma thanks for the quick turn-around!

Simple change, tests passing.

Thanks! I overlooked a couple of issues in my previous review.

penyaskito → created an issue.

penyaskito → created an issue.

Ready for another round of reviews.

There's some code standards failing on the test file. I'm guessing your editor is trying to autoformat something that it shouldn't.

Config schema looks good and MappingTest provides test coverage.

penyaskito → created an issue.

Merged into 1.x and cherry-picked to 1.1.x, thanks!

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

Created 📌 Move components listing to Appearance Active for #19, confirmed in #20.

penyaskito → created an issue.

>> 2. Also config shipped with the recipes should be translatable (is it already?).

> Yes, it should. AFAIK the missing piece here is the strings not getting extracted by potx.

I wouldn't expect changes needed at potx for shipped config, but we probably need l.d.o to parse general projects and check if they are recipes looking at composer.json?

I've rewritten some parts, hope it's more clear now. Also updated version to 11.2.0, and catch has published it.

We have a test at Drupal\Tests\layout_builder\Kernel\LayoutBuilderEntityViewDisplayTest depending on the error message.

Tested this with my MR at ✨ Provide a Config Action to add/edit a component in a layout Active and the error message is "Invalid section delta "10", there are 2 sections." as expected

kristen pol → credited penyaskito → .

penyaskito → created an issue. See original summary → .

This was fixed apparently in #2958429: Properly deprecate SafeMarkup::format() → and #1938916: Convert locale theme tables to table #type → , but it's hard to track 😅
I reviewed HEAD and definitely not an issue anymore.

penyaskito → created an issue.

Fixed feedback.

penyaskito → created an issue.

This should be ready, we still need the change record for making public setSections.

penyaskito → created an issue. See original summary → .

Test failure is unrelated.

Dupe of ✨ Provide a Config Action so recipes can add a block to a dashboard Active which is postponed on core issue. Probably nothing will need to happen on dashboard itself.

Posted from phone, sorry for brevity.

NR.
#7: Got rid of every Component1Icon. There are a couple of cases that could be discussed:
1. New component menu: Does nothing, Laurii confirmed we can delete that.
2. Not sure about my PageInfo checks, definitely appreciate feedback there @balintbrews

penyaskito → created an issue.

The block_alter is already in dashboard.
Next step for better navigation integration is at 📌 Support navigation module Active .

griffynh → credited penyaskito → .

Let us know if there's something we could document better.

Thanks!

As mentioned in #7, this is a moving target.
In 📌 Navigation leverage icon core API Needs work , the plan is to a) use icon API, b) find a way to define overrides/extend navigation icon library, while still allowing themes to override it.
In the meantime I think the safest is just fake a menu and use whatever navigation menus are using. Dirty, but works, and less change to break suddenly.

3,4,5 fixed.

6. It is not late.

> Why tweaking the dynamically generate template instead of moving the variant to the props a bit earlier?

Because with the current solution, if you already had a prop named variant because of our lack of support until now, we will be BC compatible ensuring not breaking your component (even if that's pretty unlike to break tbh). So you could have both "modern" components with variants defined and "oldest" components were you had a variant prop. Both will work, and you would have _some way_ to discern between each case.

Also I kinda like having the variant explicitly in the generated template if someone ever gets to debug this deep.

As far are I tested both solutions have the same result, but unless you have a really strong opinion on this I'd prefer having it explicitly.

kristen pol → credited penyaskito → .

Thanks! I validated the fix works for overrides too.

Gave this a quick try. Not sure how to create overrides though.

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

Created ✨ Provide an opt-out for hiding a SDC component from XB Active for following-up on Lauri's proposal.

A proposal was done in #3485820-22: Fatal error on installing the Navigation module: `PropShape::resolveSchemaReferences(): Argument #1 ($schema) must be of type array, string given` → , to add an optional property in SDCs to opt-out from XB, e.g., hideFromUi: true in core.

I've mixed feelings about doing this in core though.

It would probably be easier to implement this way, but hideFromUI could be confusing.
Something specific like hideFromXB adds a reference to experience_builder in core. What if e.g. tomorrow the storybook module wants to add a similar one?

On the other side, SDC by definition should have everything in the same directory, so there are no hook_alters.
As we already decorate ComponentPluginManager it would still be possible though to have that in XB, but breaks the _single-directory_ contract.

penyaskito → created an issue.

In February I went through this with @e0ipso when we were at the same room.
I've gone through his comments.
This still needs improvements in validation (which I might need help with) + adding more tests, but feels like could have another review.

Should we have a report on coverage with the artifacts generated, aside of the output?

This can be worked on now.

Added to menu

I've created https://www.drupal.org/docs/extending-drupal/contributed-modules/contrib... →