One of the Cypress tests is failing
1) Empty canvas
xb/xb_page/2 can add a component to an empty canvas:
AssertionError: Timed out retrying after 4000ms: Expected to find element: `[data-xb-component-id="experience_builder:my-hero"]`, but never found it.
at Context.eval (webpack://vite-template-redux/./tests/e2e/empty-canvas.cy.js:55:70)
but this is outside my skillset to fix.
longwave → created an issue.
There are also some modules that are effectively stable, but tagged as alphas for various idiosyncratic reasons
Similarly Gin has had 15 release candidates over two years, and Klaro has had 14 RCs over 18 months - surely it's time for a stable releases?
With my security team hat on I'd also like this policy to ideally define what this means in terms of security coverage. This means that although Drupal CMS 1.0 will soon have a stable release version, it seems unlikely that all of the components will actually be covered by the security team, because some dependencies currently only have alpha or beta releases. When evaluating Drupal CMS, it might be important for users to know that we provide no guarantees of security with some of the components, and we should certainly not be giving a false sense of security.
Additionally, some organisations have policies that mean they cannot use alpha or beta quality software in production. So even though they might be interested in Drupal CMS 1.0, they will be unable to use it due to the stability of the dependencies.
In the future I would really like us to aim to only use stable versions of dependencies, with security coverage, in stable versions of Drupal CMS - this isn't possible today but perhaps it is something we can aim for in the future, which would alleviate my concerns here.
Sounds like a duplicate of 🐛 ExtensionMimeTypeGuesser::guessMimeType must support file names with "0" (zero) like foo.0.zip Active , which was committed this week but not released yet?
We are already trying to squeeze too much into 11.1.0 given that we are already at rc1, so I agree with @cmlara on not committing this to 11.1.x. While this does affect Drupal CMS, given that the UI is still visible and very confusing when Automatic Updates is also installed, there is nothing stopping AU fixing that, even if that is temporary.
We still would like to remove this before Drupal 12, given that we believe few to no users currently use this feature for reasons already explained, so it is likely that this can still land in 11.2.
Thanks for trying. Given that we can't know what existing service providers might want to do, I'm not sure how we can do this without breaking backward compatibility. Maybe we just don't need to do this issue at all, and core services remain manually wired.
We could perhaps opt-in certain types of service to autowiring, such as event subscribers, which are unlikely to be altered by service providers? But unsure if there's any real benefit to that.
longwave → made their first commit to this issue’s fork.
As mentioned SectionData has this:
public function setValue($value, $notify = TRUE) {
if ($value && !$value instanceof Section) {
throw new \InvalidArgumentException(sprintf('Value assigned to "%s" is not a valid section', $this->getName()));
}
parent::setValue($value, $notify);
}
This was added in
#2926914-30: Rewrite \Drupal\layout_builder\Section to represent the entire section, not just the block info →
with no discussion or review comments so I'm not sure why it was done, but it seems feasible to extend this method to accept an array and call Section::fromArray() if necessary.
To me that is preferable to adding a special case for a module to core code.
Committed to 10.5.x, doesn't cherry-pick cleanly to 10.4.x due to conflicts in yarn.lock - although there shouldn't be any, so not sure what's happened there.
longwave → created an issue.
+1, either this is going to be an awkward upgrade path or this theme is kept around forever - it feels like the effort spent here would be better off just fixing it in core?
Somewhere/somehow we also have to explain why both "Olivero" and "Olivero for Drupal CMS" show up in the Appearance tab.
Addressed #35 by making the property nullable but the config schema still disallows it.
I don't understand how to work around
1) Drupal\Tests\experience_builder\Kernel\Entity\ComponentValidationTest::testRequiredPropertyValuesMissing
TypeError: Cannot assign null to property Drupal\experience_builder\Entity\Component::$category of type Drupal\Core\StringTranslation\TranslatableMarkup|string
Do we just need to override the entire method here? There doesn't seem to be a technique available in the parent testRequiredPropertyValuesMissing() to solve this; because core doesn't use types on config entity properties this doesn't seem to affect any core tests.
longwave → created an issue.
See #17, the problem is in the trait, PHPStan doesn't catch it if you run it on the file alone - not sure why.
Alternatively if we land 📌 Fix PHPStan L1 errors "Call to method getDefinitions()/getSortedDefinitions() on an unknown class Drupal\Core\Plugin\CategorizingPluginManagerTrait." Needs work first then the baseline changes can be removed.
Thanks, the fix is simple. Backporting this down to 10.3.x so we get the fix in all current versions of core.
Committed and pushed 3aaf8f3b86c to 11.x and 3520ae258a4 to 11.1.x and 567b629dab3 to 11.0.x and 6ad67e89ca2 to 10.5.x and c2ff9ac27f7 to 10.4.x and 0d414462a53 to 10.3.x. Thanks!
Committed and pushed to 11.x and 11.1.x, thanks!
Marking for backport to 10.4.x/10.5.x.
larowlan → credited longwave → .
xjm → credited longwave → .
xjm → credited longwave → .
xjm → credited longwave → .
It seems this was a deliberate choice at some point, from ImageItem::isDisplayed():
// Image items do not have per-item visibility settings.
This was added in #2090619: Move file visibility check to the FileItem class → but there was no discussion that I can find as to why image items should not have this setting.
However, I'm not really sure how useful this feature is. Aren't images meant to be displayed?
Saving credits.
Committed and pushed 55a11a9b9e1 to 11.x and cb5659ebac3 to 11.1.x and 3c931cde769 to 11.0.x and 8c723e205fd to 10.5.x and 7188a82cd8a to 10.4.x and 543d172a248 to 10.3.x. Thanks!
Committed and pushed cb28d50b8a8 to 10.5.x and fb7ce626967 to 10.4.x and e02f07bc153 to 10.3.x. Thanks!
Alright, let's just go with the hardened regex. The suggested changes look good to me.
longwave → changed the visibility of the branch 3492927-php-solution to hidden.
symfony/http-foundation was updated in
📌
Update Composer dependencies for 10.4.0-RC1
Active
so from here I have just committed the fix and test.
Committed and pushed c6143563c31 to 10.5.x and 3116614f706 to 10.4.x. Thanks!
Great find, thanks for the fix and test. The previous change was committed to 10.3 and 11.0 so backporting this all the way down to there as an eligible bug fix.
Committed and pushed 393baa761cc to 11.x and d83ba02ddbb to 11.1.x and bc5e491c885 to 11.0.x and 5c12effc9e3 to 10.5.x and e539d9bf68d to 10.4.x and 0475ceccefc to 10.3.x. Thanks!
Marking fixed as the betas are already out
The child issues are done, so I think this is done too?
To be pragmatic I'm committing this from needs review, given 10.4.0 is out this week, CI is green and the changes here are minimal compared to the similar but larger changes that already landed in 11.1.0.
Committed 09e9561 and pushed to 10.5.x. Thanks!
Committed a25c2c7 and pushed to 10.4.x. Thanks!
Discussed this with @f.mazeikis, @effulgentsia and @traviscarden.
We think there needs to be a checkbox to enable/disable this feature, at least for the foreseeable future, for the following reasons:
- When the user does not have a page template yet, what is the action that creates the page template? If this is an action purely in Experience Builder, that means we have to mark up at least the existing regions at all times even though the page template is not in use yet.
- When the user triggers the creation of the page template, what happens to the existing block layout?
- At present blocks can have conditions; until the XB UI has parity with the existing block UI in this regard site owners might not want to enable page templates, or if they do they need some way of switching them off again.
- We do not yet have separate permissions for content creators vs site owners, where the enabling/disabling and placing of components in non-content regions should be a site owner task.
For all these reasons it seems simplest to have a single checkbox in the theme settings. Until it is checked for the first time, the page template entity won't exist and rendering and preview will use block layout. Once it is checked then a page template entity will be created and that will be used for rendering and preview. If it is unchecked again then the page template entity will remain but the entity will not be used for rendering (haven't figured out what to do with preview here yet).
Probably we shouldn't use regex at all, as there still could be false positives if we ever use nested structures in the YAML, or heredocs, or anything similar.
MR!306 uses a pure PHP solution instead which works for me inside drupalci/php-8.3-ubuntu-apache:production.
In fact let's be bold and mark this RTBC as we both agree on the solution here.
You can see this job succeeding over at https://git.drupalcode.org/project/experience_builder/-/jobs/3648786
And failing with two deliberate mistakes added to the CSS at https://git.drupalcode.org/project/experience_builder/-/jobs/3649253
The CSS errors are correctly reported back in the pipeline "Code Quality" tab at https://git.drupalcode.org/project/experience_builder/-/pipelines/364499...
I wondered about adding the same thing after I did the initial fix, I think it's a good idea. I was trying to see if there was a way of doing it directly in sed but head is easier to understand anyway.
longwave → made their first commit to this issue’s fork.
export PROJECT_TYPE=$(sed -n -e 's/^.*type:[[:space:]]*//p' $INFO_YML)
This regex is too permissive, on experience_builder.info.yml it returns two lines:
$ sed -n -e 's/^.*type:[[:space:]]*//p' experience_builder.info.yml
module
number` etc.
Fixed by changing the first .* to [[:space:]]* as only whitespace is allowed before this YAML key.
Is something mis-parsing info.yml?!
experience_builder.info.yml
51: # For the additional examples, which use `type: number` etc.
So the issue here appears to be that build.env is invalid:
PROJECT_NAME=experience_builder
PROJECT_TYPE=module
number` etc.
MODULE_NAME=experience_builder
DRUPAL_CORE=11.1.x-dev
PHP_VERSION=8.3
PHP_IMAGE_VARIANT=ubuntu-apache
PHP_IMAGE_TAG=production
_CURL_TEMPLATES_REPO=project/gitlab_templates
_CURL_TEMPLATES_REF=default-ref
COMPOSER_END_CODE=0
No idea where number` etc. is coming from...
longwave → changed the visibility of the branch 3492235-rebuild-required to hidden.
longwave → changed the visibility of the branch 3492722-phpunit-next-major to hidden.
Well, we should now opt in to "next minor" in our CI config and then fix the failure.
I think we should do 📌 CI: update to 1.6.3 of the GitLab CI Template Active which will test against 11.1 and 10.4 and is currently green, then we can enable OPT_IN_TEST_NEXT_MINOR to add 11.2 but that's not critical to complete until closer to the release of 11.2.0.
If the default is now false we can remove container_rebuild_required: false from all .info.yml files again.
Thanks for confirming, marking this fixed as I believe that is the solutionm for all affected sites - there is nothing we can do in Drupal core to help with this.
@jonathan1055 The XB case is slightly more complicated by the fact we are still on v1.5 of the templates. I opened 📌 CI: update to 1.6.3 of the GitLab CI Template Active to solve that first, which is now green except for Stylelint. I then opened 📌 Fix CI for Stylelint 16 Active which is the changes from that MR plus a pointer to this MR to test the Stylelint fix, but the composer step fails and I'm not sure why.
📌 Install modules with container_rebuild_true set by themselves Active landed so this should no longer be necessary - @tedbow can you confirm?
We should still do 📌 CI: update to 1.6.3 of the GitLab CI Template Active .
Wondering if we can make things even easier by assuming contributors will use ddev and @traviscarden's add-on, which makes installation even easier: https://github.com/TravisCarden/ddev-drupal-xb-dev?tab=readme-ov-file#in...
Thanks, I'm learning more about config schema from you in every MR!
Merged in 0.x, unassigning so someone else can review.
Hm, OK. Felix and I discussed this previously and I thought we came to the conclusion that a checkbox setting was going to be needed somewhere, but perhaps this will work. Will switch tack to using the config entity status for now.
I still think in the future that a single page template per theme is going to be too limiting, but for now we can live with it.
As per this GitHub comment it's likely that if you are suffering from this after updating then you need to also clear your APCu cache - the easiest way to do this is to restart your webserver or PHP-FPM.
Experience Builder got a new test fail on 11.x HEAD, this issue fixes it. See 🐛 PHPUnit Next Major tests failing Active
The change here makes sense to me, so marking RTBC.
📌 Install modules with container_rebuild_true set by themselves Active indeed does fix it.
I can reproduce the fail locally with
$ ddev xb-set-core-version 11.x-dev
$ ddev test tests/src/Kernel/Plugin/Field/FieldType/ComponentTreeItemTest.php --filter testInvalidField@valid.*props
1) Drupal\Tests\experience_builder\Kernel\Plugin\Field\FieldType\ComponentTreeItemTest::testInvalidField with data set "valid values using static props" (['{"a548b48d-58a8-4077-aa04-da9...ts"}}}', '{"dynamic-static-card2df":{"h...ue"}}}'], [])
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
-Array &0 []
+Array &0 [
+ 'field_xb_demo.0' => Array &1 [
+ 0 => 'The array must contain a "tree" key.',
+ 1 => 'The array must contain a "props" key.',
+ ],
+]
/var/www/html/web/modules/contrib/experience_builder/tests/src/Kernel/Plugin/Field/FieldType/ComponentTreeItemTest.php:233
📌 CI: update to 1.6.3 of the GitLab CI Template Active also pins to 11.1.x, maybe worth doing that instead as Stylelint is also failing but upgrading the template will eventually unblock that via 📌 Update Stylelint formatter for Stylelint 16 Active
Merged in 📌 CI: update to 1.6.3 of the GitLab CI Template Active , this issue is not for merging just for proving the fixes work in combination.
The Stylelint job fails, but that fails already; this needs fixing over in 📌 Update Stylelint formatter for Stylelint 16 Active (see 📌 Fix CI for Stylelint 16 Active for a test of that fix)
Yeah, I just found out/remembered that Experience Builder is pinned to v1.5 of the templates, we need to upgrade in 📌 CI: update to 1.6.3 of the GitLab CI Template Active . Not sure if we are doing anything else on top to force use of 11.x.
But this issue will presumably hit other projects as soon as they start testing against 11.1.0, which is due this week.
@bnjmnm reported Stylelint was breaking in recent CI runs, this should be fixed by 📌 Update Stylelint formatter for Stylelint 16 Active but we also need to complete this now.
Blocked by 📌 CI: update to 1.6.3 of the GitLab CI Template Active
longwave → created an issue.
I think it will only break MRs that run against core 11.x where Stylelint was upgraded, this change has yet to land in 10.4.x.
The release notes are for things that affect site owners, but this is worth calling out as a highlight in the announcement for 11.2.0.
Works if I run the same commands locally, unsure how else to test.
longwave → created an issue.
Started this and immediately run into 🐛 third_party_settings in THEME.settings.yml cannot be dependency managed Needs work
Perhaps for now we just keep the setting at the top level of THEME.settings.yml.
longwave → created an issue.
ModuleHandler::buildModuleDependencies() already builds a graph of dependencies so I was hoping we could use that.
I think that we need install modules in the order of the longest possible route through the graph, which will ensure that all dependencies are met by the time each subsequent module gets to be installed.
Committed and pushed 8bdb090a3db to 10.5.x and a026efe5199 to 10.4.x. Thanks!
Given this is quite a big change and we are already in RC for 11.1 this is only eligible for commit to 11.x and will be in 11.2.0. This gives us a few months for contrib to add the new flag or (if possible) for us to auto detect situations when the container rebuild is actually required.
Committed f8090e6 and pushed to 11.x. Thanks!
Also published the CRs.
MR needs rebasing, plus the other "needs" tags must be completed before this can be RTBC.
Some tests are being super flaky at the moment, not sure why.
I have been seeing multiple random fails on many core MRs recently, up to four jobs failing but that pass on retry, so not sure this is any worse.
I haven't looked but I don't understand in the config_selector case how it ensures it is installed first when multiple modules are installed simultaneously, if it affects the install process itself.
Building on #3, maybe we can just rely on dependencies? If we build a graph of dependencies then we can guarantee container rebuilds between two modules that depend on each other, but we can still combine the branches of the graph in a way that minimises the total number of rebuilds that is required.
If this works then all that modules have to do is ensure their dependencies are correctly declared.
// Let other modules react.
$this->invokeAll('modules_uninstalled', [$module_list, $sync_status]);
// Flush all persistent caches.
// Any cache entry might implicitly depend on the uninstalled modules,
// so clear all of them explicitly.
$this->invokeAll('cache_flush');
would be much clearer to me as
// Load all modules because ...
$this->moduleHandler->loadAll();
// Let other modules react.
$this->moduleHandler->invokeAll('modules_uninstalled', [$module_list, $sync_status]);
// Flush all persistent caches.
// Any cache entry might implicitly depend on the uninstalled modules,
// so clear all of them explicitly.
$this->moduleHandler->invokeAll('cache_flush');
Similarly
foreach ($module_list as $module) {
...
// Allow modules to react prior to the uninstallation of a module.
$this->invokeAll('module_preuninstall', [$module, $sync_status]);
Why do we need to call loadAll() every time we go around the loop? Clearer to call it once at the start, with a reason.
longwave → created an issue.
For #4 I'm trying to think how we could automatically detect that but it's tricky because the decoration is done in a service provider class.
Wondering if we can do this in a safe way. If a module calls \Drupal::service() in hook_install() for a service provided by a just-installed dependency then we need to rebuild the container, but that's the problem of the second module - the container only needs to be rebuilt if both are installed at the same time.
Do we know what other scenarios require a container rebuild?
Yeah, I think this is a false positive, it's not token_get_all() that's the problem - rebuilding the container does leak memory somewhere, but this isn't it.