I think forcing data loss with no warning is surprising behaviour and should be avoided. At least we should ask "we are going to reinstall Experience Builder, are you sure you want to continue" - maybe just drop the --yes from the uninstall?
Also this won't work for people like me who run Drupal inside ddev but who also run composer on the host, there is no workaround for that I guess.
// Delete all xb_page entities and uninstall XB.
$run_drush(['entity:delete', 'xb_page']);
$run_drush(['pm:uninstall', 'experience_builder', '--yes']);
Is this guaranteed to uninstall Experience Builder? What happens if there are other dependencies on XB - let's say someone has built a view of XB pages? I feel like this risks data loss if we are just force uninstalling the module with no questions asked.
longwave → changed the visibility of the branch 3491459-create-api-slice-for-pending-changes to hidden.
MR!577 implements #24.1/#24.2 and the comment in #25.
- Pages have a grey page icon.
- Global regions have a green cube icon.
- Nodes (CMS content) have a grey cylinder icon.
The last one wasn't available in Radix so I exported the SVG from Figma and added vite-plugin-svgr to be able to use SVGs as React components - looks like this is the first time we have needed to do this.
Americanizing the issue title.
Opened 📌 Decide how to assign colours to users for "review changes" Active for #22.4 / #24.4.
I think we could finish off #22.1 and #22.2 in this issue.
longwave → created an issue.
This appears to be duplicating what is being done in core in 📌 [PP1] Show entity information on the Top Bar Postponed
I don't think this is strictly dependent on the meta. It is important to get done and it's also related to landing the code components, because they will need a third type of props form (albeit likely very similar to the SDC one), not sure if we should unify them all before or after that lands.
Although I would much prefer to mark this as closed (won't fix), MAINTAINERS.txt should reflect reality.
Thank you for your numerous contributions, I echo @larowlan in saying that Drupal wouldn't be what it is without your efforts.
Also looked at this a bit today. The issue is the additional JsComponentHasValidSdcMetadata constraint isn't compatible with ConfigEntityValidationTestBase; for an invalid ID we report multiple errors:
'' => [
0 => '[id] Does not match the regex pattern ^[a-z]([a-zA-Z0-9_-]*[a-zA-Z0-9])*:[a-z]([a-zA-Z0-9_-]*[a-zA-Z0-9])*$/n[machineName] Does not match the regex pattern ^[a-z]([a-zA-Z0-9_-]*[a-zA-Z0-9])*$',
1 => 'The 'id' property cannot be changed.',
],
'id' => 'The <em class="placeholder">"space separated"</em> machine name is not valid.',
whereas the test only expects
'' => 'The 'id' property cannot be changed.'
It would be nice to use the Sequentially validator to handle this - so the immutable properties would error first, and then we only validate SDC metadata if there are no other errors, but this isn't supported in core. I tried to hack it in but ContextDefinition::getConstraintObjects() handles converting the constraint plugin IDs to objects and doesn't handle nesting.
The easiest fix is to live with the more awkward error messages for now and change the test instead.
Duplicate of 🐛 PHPUnit Next Major tests failing Active ?
🐛 PHPUnit Next Major tests failing Active should enable a job for this.
While we are here we should review ignoreErrors in phpstan.neon and either convert them to inline ignores, or even better, fix them.
@traviscarden discovered this initially.
longwave → created an issue. See original summary → .
For me the issue was ✨ DomPDF remote images blocked/broken due to missing user agent Needs work - if you have Cloudflare or a similar WAF in front of your site that can be the cause of missing CSS and images.
I ran into the same issue and came to the same conclusion but found my own solution before finding this issue.
I don't see why this needs a configuration option. MR!69 proposes setting the user agent in a similar way to how core sets it for Guzzle.
Alternatively, Dompdf does set this by itself, but we blindly overwrite the entire stream context; an alternative approach would be to read the stream context and update the options instead of overwriting the whole thing.
Anyway, MR!69 is applied on one of our sites and works where images were previously blocked due to no user-agent header.
longwave → made their first commit to this issue’s fork.
Fixed the failing test, added a minor comment based on an MR comment, otherwise no further feedback on this - I think it's ready to go.
Regarding the functional/kernel split ✨ Add drupalGet() to KernelTestBase Active would give us the best of both worlds, but unsure if we should borrow parts od that here or ignore it until it actually lands in core.
@wim leers I discussed this a bit on standup with @lauriii. We came to the conclusion that "global CSS" (which perhaps needs a different name) should be stored separately from page templates. Initially there will be a 1:1 mapping of global CSS to themes, the same as page templates, but perhaps in the future you might want multiple CSS files, in which case we would need to extend that out. Also, to the site owner, editing global CSS will be done separately from editing the page templates, so it seems to make sense to keep the concepts separate on the back end as well. As #14 states you might want to use global CSS without enabling page templates for the current theme; the global CSS will be applied whether or not a page template is currently in use.
longwave → made their first commit to this issue’s fork.
> We may want multiple global stylesheets in future
I don't get why we are saying this here, but previously we stated that we would only ever have one page template per theme. To me a global stylesheet ties in with a global page template. Back then I could see that I might want multiple page templates - some "special" pages might want to opt in to an entirely different template via some mechanism - but we ruled that out, so why aren't we doing the same for global CSS?
Until the way ahead and the future requirements are clearer, I still think the simplest option is to add this as an additional property to the page template config entity. As stated in #8 we can always migrate CSS from the page template to somewhere else. It's easier to add a single property to an existing config entity and move it later, than it is to define a whole new config entity and refactor/remove the whole thing later.
I think @larowlan's suggested renaming/refactors can be done in followups, but added a minor refactoring of my own to make the JS easier to follow and added some questions/suggestions.
Firstly, the split between kernel and functional tests. Before I was in favour of using kernel tests because they were faster, but now we moved error handling to an event listener, calling the controller directly is not enough any more - so functional tests are more realistic. We should decide on the correct way and use it consistently.
Also, now we have the CSRF token we need to provide that with most API calls. Other than explicitly testing that the token is required, tests shouldn't need to care about the token itself, this should be hidden from the test author in most cases - having a common API request method that handles this for us would be useful.
quietone → credited longwave → .
longwave → created an issue.
longwave → changed the visibility of the branch 3497695-convert-apicontrollerbasecreatejsonresponsefromviolationsets-to to hidden.
Rebased.
It's green!
Should this be part of the page template entity, given we currently have one of those per theme?
@andypost I read that as: if it's -1 you cannot change it, but otherwise you can swap between 0 and 1 at runtime?
Self-RTBCing as this is a one liner that doesn't affect us.
longwave → created an issue.
It might equally work by adding
ini_set('zend.assertions', 1);
ini_set('assert.active', 1);
to settings.local.php, but needs testing. zend.assertions cannot be set to 1 if it was -1 in php.ini, but as we control the environment that will never be the case here.
This might not be necessary if we fix in core via 📌 Remove mentions of assert.active from .htaccess Needs work , although existing tests against current versions of core are running with no assertions so we might still want to fix that.
Should we add
if (ini_get('zend.assertions') !== -1) {
ini_set('zend.assertions', 1);
}
and even though it's deprecated maybe even
ini_set('assert.active', 1);
back to DrupalKernel::bootEnvironment() when we detect we are running in a test environment? This would attempt to force a more consistent environment in tests.
Please credit @wim leers for contributing to the discovery and suggested fix for this issue.
longwave → created an issue.
The MR is no good, enabling them in PHPUnit isn't the problem, it's in the site under test that we need to do it.
longwave → made their first commit to this issue’s fork.
The issue with component-operations.cy.js is
'static-static-card1ab' => [
'heading' => [
'sourceType' => 'static:field_item:string',
'value' => 'hello, world!',
'expression' => 'ℹ︎string␟value',
],
'cta1href' => [
'sourceType' => 'static:field_item:uri',
'value' => 'https://drupal.org',
'expression' => 'ℹ︎uri␟value',
],
],
Because we only have these two props set in the stored model, only these two fields actually render in the SDC component form. Not sure where to fix this, putting this down here for today.
LGTM, I like the additional changes in MR!547 - RTBC once the nits from the review are done.
longwave → changed the visibility of the branch 3484678-improve-or-remove to hidden.
Do we actually want to do this now, or postpone until we are about to remove Ban?
longwave → created an issue.
Opened 📌 Handle all paths in OpenAPI request validation Active as a first step.
longwave → created an issue. See original summary → .
Some nitpicks but the proposal looks good to me in principle.
It's a minor mistake - I was testing against 3.16 and put that instead of 3.15 - but not sure it really matters? drupal/core-recommended for 10.4 and 11.1 already both depend on twig/twig:~v3.16.0 and Composer will just sort it out anyway. Instead I think we should set the minimum version of core for XB to be 10.4/11.1, in a new issue.
Oh wow, this is a trivial DX improvement we should make in core to avoid others running into this issue, there's no reason why we should accept the prefixed controller and then fail with a mysterious error - it should either work, or error immediately.
longwave → made their first commit to this issue’s fork.
Been going back and forth to try and figure out the best way forward here. The problem is that the SDC version of the form presents field widgets, and these have keys from field widgets, e.g. heading[0][value], which we then map into props. Block forms (and any other non-widget form I guess) just has plain keys, e.g. level. But we can't make assumptions, because there might be complex block forms that have structures that look a bit like widget forms, but aren't - so we do need to distinguish between them somehow - but right now these Form API structures have specific assumptions made about them.
As far as I know there is no requirement to have block forms have dynamic props, we can assume static values. But not sure about other component source plugins, they might still want to support dynamic props?
We should remove patternProperties from openapi.yml if it doesn't work, but otherwise I am not sure we need to strictly validate here, Drupal will do it for us anyway?
I learned enough React to be able to hack something together here that seems to work. Block settings are a hybrid of SDC component forms (as they update the model) and page data forms (as they use standard Drupal forms), I think inputBehaviors needs some more refactoring to figure this out to how split this up in a better way.
Fix was committed upstream, nothing to do here.
longwave → created an issue.
Also I think this should be in a separate xb_node submodule to keep the code entirely separate, and remove the checkbox from sites that never want to use XB on nodes?
MR!526 looks like a good start but I am wondering whether these should be configurable fields, because we don't want the user to be able to delete them once they have checked the checkbox. For pages we define a components base field, and I'm wondering if we should do the same for other entity types, except as bundle fields (because they don't necessarily appear on all bundles) via hook_entity_bundle_field_info().
@effulgentsia I think we should at least label it as lifecycle: experimental.
longwave → created an issue.
Ah that makes sense but I also think that all these component listings/groupings should be 100% controllable from the server side, we shouldn't have any hardcoded restrictions - we should be flexible in allowing developers to customise the XB UI from PHP wherever possible. This means that even if the library and top bar have groupings (like the Sections/Components split we have at the moment), the number/names of those groups should be sent from the server.
Agree with Wim - big -1 for this, this feels like a huge can of worms to open as we load those config entities in many places and presumably that would all have to be refactored, and config dependencies should just solve the problem for us.
#17 is down to plurals - "field" vs "fields" in the error message now we have 1 vs 2 fields instead of 2 vs 3, along with assertUninstallFailureReasons() using lax comparisons via assertStringContainsString(). Tightened that up here to use assertSame() instead and specified the exact error messages.
We also can't depend on a profile from a module, and even if we could then we can't guarantee the article type exists. I think better just to make assumptions here for now (as we did before) and spend efforts on expanding XB to support other node bundles via a proper UI, bundle fields, etc.
I started looking at getting rid of this special case but we quickly run into 📌 Finalize API for creating, overriding, and altering code-defined bundle fields Needs work - it would be great if we could just provide a single named bundle fields instead of configurable fields for XB which would simplify things elsewhere but this is non trivial as there are unfinished parts in core.
We need more information to be able to reproduce this.
longwave → made their first commit to this issue’s fork.
Added some rudimentary documentation.
While we're here what is the end goal for nodes? A per-bundle checkbox somewhere that enables Experience Builder for the bundle? (a bit like the LB one I guess, except at the node level instead of the display)
Updated Tugboat config, also opened https://github.com/TravisCarden/ddev-drupal-xb-dev/pull/25 for developers using @TravisCarden's DDEV addon.
Took the liberty to start this given we want it for 0.2, this will probably break all the tests.
longwave → made their first commit to this issue’s fork.
One nit to appease PHPStan but otherwise this looks good to me.
#24 seems feasible for the solution to attributes although I still think injecting them in the right place on the server side is going to be tricky in some cases.
The sample in #25 doesn't work like you expect:
> document.querySelector('input').type
"text"
and it renders in the browser as a text input, not a date.
My bad, this is an oversight in 📌 Throw exceptions instead of returning tuples for validation errors Active .
Previously we would return the structured tree and try to store it, even if it was invalid, because we need to be able to preview it still. A mistake in that issue means we return NULL on an invalid tree instead.
We can work around this by making the validation optional in the preview case, until 📌 Improve server-side error handling Active lands which there is already a @todo for.
longwave → made their first commit to this issue’s fork.
I don't see what's wrong with using the official NodeJS image? It's available for multiple architectures and does what we need - what does the skpr one do that the official one doesn't?
MR!515 contains a first attempt at a Docker-based build step that can be run locally or on CI.
$ rm -rf dist
$ docker build --output dist .
...
=> exporting to client directory
=> => copying files 1.62MB
$ ls dist/ dist/assets/
dist/:
assets index.html
dist/assets/:
index.css index.js
📌
Add JavaScript build artifacts to tagged releases
Active
is a blocker assuming we don't want Drupal CMS users to be manually running npm run build before they can use XB.
Adding credits for discussion in Slack.
longwave → created an issue.
#21 requires that we are able to annotate element, attribute and text nodes in the HTML, and I don't disagree with this; we need to be able to reference any of these in XB. However, elements and text can be wrapped by comment nodes, but attributes cannot - comments are not allowed inside tags. Which I think means we need another solution for attributes anyway?
So given that I'm wondering if there is another solution here that doesn't require wrapping at all. Maybe trying to keep the XB metadata inside the document is not the right answer - should the metadata be out of band instead? Could we have a separate JSON structure that maps UUIDs/other data to some kind of querySelector/XPath style mechanism that can refer to a specific node in the DOM, no matter whether that's an element, attribute or text node?