- 🇫🇷France nod_ Lille
I'd like to have a function to avoid duplicating the code in two places here. The function can be local to this file, no need to add something to the global
Drupal
object - 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
Follow-ups:
- #19.1: 📌 [later phase] [PP-1] When the field type for a PropShape changes, the Content Creator must be able to upgrade Postponed (and I realized it had a blocker itself: ✨ [later phase] PropShapes' JSON schema must match exactly with FieldType's storage format — what if you want to use another FieldType? Active ), and then the 3 at the bottom of #17:
- 📌 [PP-1] Auto-generate Component config entities for all discovered SDCs that meet XB's minimum criteria Active
- 📌 [PP-2] Remove Component config entity's add/edit form Postponed
-
📌
[PP-1] Introduce `FieldTypeStorageForJsonSchemaDefinition` plugin type
Postponed
Bonus follow-ups: 📌 Component config entity should validate that the SDC actually (still) exists Active + 📌 [PP-2] Add ComponentAuditabilityTest Active .
I'll continue working on this MR on Monday. Now: time for blog posts :)
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
Discussed #17 with @lauriii. His replies:
- Yes, but needs follow-up to devise a UX to allow existing
StaticPropSource
s using a previous field type+widget to update to the new one. - Yes
- Yes
He agreed with the 3 follow-ups proposed at the bottom of #17.
We also discussed #18, and he prefers the "alternatively" — so repurposed that 👍
P.S.: Note that this also blocks 📌 Create a component for testing form backend + frontend integration Active .
- Yes, but needs follow-up to devise a UX to allow existing
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
Oh, and once this lands, 📌 [PP-1] Media Library integration Postponed becomes trivially unblocked: it'll only require:
- new configuration to allow declaring a preference for
image
vsmedia
(which oddly would not be possible through a newexperience_builder.settings.yml
config, only through a new config entity, because simple config cannot have module nor config dependencies) - updating
\Drupal\experience_builder\SdcPropJsonSchemaType::findFieldTypeStorage()
to respect that configuration
Those 2 things would be in an MR for that issue which I could start and then the front-end team could continue.
Alternatively, I could repurpose ✨ Hardcode image props to use the media widget temporarily Active for that, so that #3454173 can stay focused on the bigger picture, of making that media library widget actually work (well).
- new configuration to allow declaring a preference for
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
The
\Drupal\experience_builder\Form\ComponentEditForm
UX became simpler:Discussion with @lauriii
Yesterday morning, prior to writing #15, @lauriii and I met and discussed this problem space in detail.
The key outcomes of that discussion:
- XB will require that an SDC provides an example value for it to be surfaced in XB. IOW: the
examples
array must not be empty, and XB will use the first one. ⇒ Removes the need for 25% of what theComponent
config entity provides. - @lauriii was heavily leaning towards "no UI" and "logic with an alter hook".
- Tricky: complex shapes, like
json-schema-definitions://experience_builder.module/image
— those would need a plugin system to provide the appropriate field type + field storage.
This issue: current MR state and current consequences
This issue proves it's possible to generate the default
field_type
,expression
andfield_widget
value for each prop. ⇒ Removes the need for 75% of what theComponent
config entity provides.This issue did implement it all as logic (i.e. with nothing stored as a config entity — see my comment #15 for why that turned out to be trickier than I thought when I created this issue), with a TODO for an alter hook.
As demonstrated in #16, this means the scope for 📌 [PP-1] Add support for matching SDC prop shape: {type: string, enum: …} Postponed changes to only having to worry about matching existing field instances that can be adapted to match the SDC prop's enum values.
Questions for @lauriii prior to landing this MR
- Is it acceptable for the logic that determines which field type + widget to use to change and hence newly created instances of this component to potentially use a different field type than the pre-existing component instances?
- SDC supports default
*.component.yml
-defined values. But that won't work for e.g. a default image. Is it okay for XB to layer on support for that by auto-discovering apropname.(gif|png|jpg|…)
file in the SDC's directory? 🤔 (Will add this to 📌 [SPIKE] Comprehensive plan for integrating with SDC Needs work , to potentially add that to upstream SDC.) - Is it acceptable that the default value sourced from a component's
examples
cannot be overridden on a per-site basis?
I think the answer to the first question is "yes". (Note that every stored
StaticPropSource
is self-contained: the user will continue to be able to edit it, even if it's a completely different field type.)If the answer to the second and third questions is "yes", then we can in principle remove the
Component
config entity. But that would introduce a huge risk: it'd make exporting/sharing configuration (in the future: 🌱 [later phase] [META] 7. Content type templates — aka "default layouts" — affects the tree+props data model Active , but now:config/optional/field.field.node.article.field_xb_demo.yml
, which has config dependencies on theComponent
config entities it uses) brittle.Therefore I think it'd be preferable to keep
Component
config entities under the hood — without showing any of that in the UI. I think that would also keep the door open for supporting per-site default values in the future, plus it will likely help with 🌱 [META] Support component types other than SDC Active .If you agree, then I'll create three follow-up issues:
- one that generates
Component
config entities for all discovered SDCs 1) upon installing XB, 2) upon installing a new module or theme - one to remove
\Drupal\experience_builder\Form\ComponentEditForm
after that auto-generation is happening - one for computing the appropriate field type + storage for
$ref
-defined prop shapes
- XB will require that an SDC provides an example value for it to be surfaced in XB. IOW: the
- 🇳🇿New Zealand quietone New Zealand
I read the IS summary, the comments and the MR. The proposed resolution is out of dateAll questions are answered and the threads in the MR are correctly resolved.
I then applied the diff and tested. This works as expected and is an improvement!
I do think there follow up work.
- Add this feature to media files as well.
- The warning message uses the string 'It is advised' which is the first occurrence in core for a warning message. Because of that I think that the usability folks should review the string.
- The first time I read "It is advised to store file uploads for contact forms as private files. You can configure this in settings.php" I read the first as something to action so I tried to. Of course, the option is not available and I knew that but I still tried! So, again, I think the a usability review would help.
I think the above items can be done in followups. I am adding the tag for that.
I finally read the test. Is there a reason it is only testing 1 code path?
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
I started with a test that gathers all unique SDC prop schemas. Then I started writing logic to automatically suggest the sane core-provided field type choice.
In doing so, I started treading on the territory of #3456008 — and I believe that we now should not do that issue anymore, at least not in its original scope: 📌 [PP-1] Add support for matching SDC prop shape: {type: string, enum: …} Postponed .
I am now thinking that not using a config entity type might actually be the only viable choice, because the config entity being proposed here only is viable if there is a good way to transform a given
SDC prop schema
into aPropShapeInput config entity ID
— because that's how we could achieve sane dependencies.The only reason that worked in the original proposal, is because HEAD does not yet support the
{type: string, enum: …}
use case, which is AFAICT widespread among SDCs. Once that is considered, the only way to generate a config entity ID is by hashing the enum.The issue title + summary need a drastic update. See the new test class, which has 3 test methods that build on top of each other. I'll continue this MR tomorrow and will update this issue as well as 🌱 [META] Early phase back-end work coordination Active .
- 🇳🇱Netherlands Spokje
Tests are still green.
For the record, just removed the
resolutions
-section and did a$ yarn install
. - 🇳🇱Netherlands Spokje
I think we can now also drop the
resloutions
section fromcore/package.json
.
These were only needed to keep the dependencies of the old version ofnightwatch
clashing with our other JS-dependencies. - 🇬🇧United Kingdom longwave UK
I'm slightly amazed that it was only another call to
execute()
that needed fixing, but the run is green. - @wim-leers opened merge request.
- 🇬🇧United Kingdom longwave UK
Last run was looking promising until it appeared to hang after installProfileTest: https://git.drupalcode.org/project/drupal/-/jobs/2214554
- 🇬🇧United Kingdom longwave UK
api.execute()
takes the callback as the third argument instead of the second now, it seems. - 🇬🇧United Kingdom longwave UK
Locally I bumped to Nightwatch 3.7.0 which works OK with the new Selenium setup and latest Chrome, but tests appear to crash and then the Selenium server no longer responds:
$ yarn test:nightwatch tests/Drupal/Nightwatch/Tests/loginTest.js [Tests/Login Test] Test Suite ────────────────────────────────────────────────────────── ℹ Connected to selenium on port 4444 (505ms). Using: chrome-headless-shell (126.0.6478.114) on LINUX. ℹ Loaded url http://ddev-drupal8-web in 446ms Running Test login: ─────────────────────────────────────────────────────────────────────────────────────────────────── ℹ Loaded url http://ddev-drupal8-web/user/reset/1/1721811628/FNZSmygKgxaar4E9AmDAlZ75Q61bU4ew0iwBaov3XUA/login in 440ms ℹ Loaded url http://ddev-drupal8-web/admin/people/roles/add in 133ms ✔ Expected element <.user-role-form .machine-name-value> to be visible in 2000ms (36ms) ✔ Testing if element <[data-drupal-messages]> contains text 'Role xt4aw4gms9 has been added.' (24ms) ℹ Loaded url http://ddev-drupal8-web/admin/people/permissions in 160ms ✔ Element <table.permissions> was visible after 34 milliseconds. ✔ Testing if element <[data-drupal-messages]> contains text 'The changes have been saved.' (22ms) ℹ Loaded url http://ddev-drupal8-web/user/logout/confirm in 69ms ℹ Loaded url http://ddev-drupal8-web/user/reset/1/1721811630/RH7RHeQ6JGfOHJ7RGMIXnuu3wYDMoU3KEqDnBTZrHfI/login in 83ms ℹ Loaded url http://ddev-drupal8-web/admin/people/create in 135ms ✔ User "user" was created successfully (34ms) ℹ Loaded url http://ddev-drupal8-web/user/logout/confirm in 47ms ℹ Loaded url http://ddev-drupal8-web/user/login in 54ms ✔ Passed [equal]: The user "user" was logged in. ℹ Loaded url http://ddev-drupal8-web/admin/reports in 76ms ✔ Element <body> was visible after 30 milliseconds. ✔ Testing if element <h1> contains text 'Reports' (32ms) $
I was expecting further output here, if I run this on the older version of Nightwatch it ends as follows:
✔ Testing if element <h1> contains text 'Reports' (29ms) ✔ Ensuring no deprecation errors have been triggered (10ms) ✨ PASSED. 9 assertions. (3.879s) Wrote HTML report file to: /var/www/html/drupal/core/reports/nightwatch/nightwatch-html-report/index.html $
If I then run the test again it hangs:
$ yarn test:nightwatch tests/Drupal/Nightwatch/Tests/loginTest.js [Tests/Login Test] Test Suite ────────────────────────────────────────────────────────── ⠴ Connecting to selenium on port 4444...
Restarting the Selenium container fixes it.
Will push this branch up anyway but not sure what's happened here at all.
- 🇬🇧United Kingdom longwave UK
📌 Use selenium/standalone-chrome instead of our chromedriver image Needs work landed which gives us a newer Selenium/Chrome to talk to and which may solve the issues we were seeing here.
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
@lauriii is not yet convinced, but he's not available to discuss it further, so to A) get this moving, B) ground the conversation with @lauriii more, getting started on this nonetheless.
- 🇮🇳India riddhi.addweb
I have applied the MR !17 and it resolves all the errors.
Please check the screenshot for the same. - 🇦🇺Australia jannakha Brisbane!
patch #466 applies to D10.3, but selected image styles are not visible in CKEditor 5 while adding/editing image:
Config works as expected:
Selected image styles are not available in CKEditor5: