🇬🇧United Kingdom @longwave

As per #3492523-40: Class "Doctrine\Deprecations\Deprecation" not found → while this does automatically invalidate the container, I'm not convinced that it affects the autoloader.

Yeah, I don't think deployment_identifier will have any effect on this. I think only the list of enabled modules is considered. From DrupalKernel::boot():

    // Add the APCu prefix to use to cache found/not-found classes.
    if (Settings::get('class_loader_auto_detect', TRUE) && method_exists($this->classLoader, 'setApcuPrefix')) {
      // Vary the APCu key by which modules are installed to allow
      // class_exists() checks to determine functionality.
      $id = 'class_loader:' . crc32(implode(':', array_keys($this->container->getParameter('container.modules'))));
      $prefix = Settings::getApcuPrefix($id, $this->root);
      $this->classLoader->setApcuPrefix($prefix);
    }

We could add deployment_identifier to that cache key?

I was about to post that I think we can and should remove all this without deprecation, but after spending a long time in GitLab search without finding anything useful except duplicate copies of core, I did spot that Ludwig → uses some of the FileTransfer classes, and the module has 11,000 users so I'm not sure we can just drop all of this.

In terms of the UI we believe that nobody is using these features; we don't get any bug reports or support requests about them that I have seen. We do still have users that update without Composer - I see them in the issue queue from time to time - but they still don't use this UI.

Perhaps we can split this into two issues? One to decide what to do about the strictly UI related parts - either we disable a lot of it and add deprecation notices, or outright remove it - and a followup where we deprecate FileTransfer and some of the related parts?

MR 495 fixes the test fails and also ensures that this won't happen again by forcing PHPUnit to run if any Twig files are changed.

This broke the tests.

I've tied this in with 📌 Add setting to enable XB for page templates Active and ensured that:

• If the page template is enabled, we send all regions to the layout and preview endpoints
• If the page template is disabled or doesn't exist, we only send the content region

The documentation for updating core manually does recommend deleting files, see step 4 of https://www.drupal.org/docs/updating-drupal/updating-drupal-core-manuall... →

Given the discussion in #7-11 I still think the preview endpoint is the best place to add this to the API.

I suggest adding the title to the JSON response in XBPreviewRenderer::renderResponse(); it looks like we might be able to capture the title in prepare().

When you update an installation without composer, do you delete any files?

The reason I ask this is that 📌 Remove adding an extension via a URL Fixed deleted core/modules/update/update.links.action.yml which contained references to these routes, but that should have been deleted when you upgraded to 10.4.0.

FWIW core excludes Drupal.Commenting.VariableComment.Missing for tests, perhaps we should do the same, but keep it for runtime code as it can be useful to actually document things.

Committed and pushed 5e89fa50893 to 11.x and 7ba9c1951c7 to 11.1.x. Thanks!

Those routes were deleted in 10.4.0 as part of the change record described above, but there is no reference to them remaining in the codebase. Do you have any modules that extend the update system that still refer to them? Or perhaps are there any references to them in your database somewhere?

Perhaps we have to revisit this and retain the routes for now but deprecate them instead, though that is currently not possible: 📌 Support route aliasing (Symfony 5.4) and allow deprecating the route name Needs work

longwave → created an issue.

Do you mean you got those errors after updating the code, but without running update.php or clearing caches? If so that sort of thing is to be expected, after updating code you must immediately run update.php to get things in sync for the new version. If there are no errors after running update.php, then everything would seem to be OK.

The collapsed output looks good to me - will have to remember we can do this for other jobs!

🐛 Uncaught ajax.js error / exception Active is possibly fixed by this as well.

This is neat, but it does hide the real problem. I thought about a different solution when I ran into this before - leading whitespace in a .module file - but never implemented it: when we include .module or .theme files, we could wrap them in ob_start() and ob_end_clean(), and optionally throw a warning to tell developers that there is a problem with their file?

I think an error has been introduced in the "show environment variables" section?

Whoops, thanks for spotting that.

Committed 74b9039 and pushed to 11.1.x. Thanks!

This commit will show up in the original issue rather than here as I cherry-picked it and it has the original issue number: 🐛 Reinstate drupal_common_theme() and deprecate it Active

larowlan → credited longwave → .

Naming things is hard when you have multiple layers of indirection and similar things that are related to each other, as per both this issue itself *and* the title.

Opened 📌 Rename component source plugin configuration keys for clarity Active for #38.5.

longwave → created an issue.

Rebased following 📌 Add support for global regions Active being merged.

This has 📌 Add support for global regions Active merged in and will make more sense once that has landed in 0.x.

At present autosave and reload of page templates seems to work, next steps is to actually render the autosaved page template, plus the saved model needs adjusting to extract only the parts relevant to the content entity and page template entity instead of saving the entire thing.

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

Not sure we need explicitInputName both as a const and in the plugin metadata; we should pick one only.

longwave → changed the visibility of the branch 3489899-add-global-regions to active.

> Recipes can only install config or perform config actions.

So let's just use config? We don't have to provide UI, and in a future version of XB when we want to remove the flag, we can force-remove the config via an update hook.

Looks great to me. Assigning to Wim as code owner for CI.

It seems that the output is written to stderr (not stdout) as well as the specified --output-file. This doesn't seem to be configurable that I can find.

We could add 2>/dev/null to hide the output but I wonder if that risks also hiding real error messages?

longwave → created an issue.

These aren't real hooks anyway, they are invoked individually for the theme engine. 📌 Convert theme engines into services Needs work would likely remove them all and convert them to methods on service classes.

I think the warnings are safe to ignore.

I fixed the errors by removing the two sets of acorn data from yarn.lock and rerunning yarn install which has deduplicated them correctly and yarn check now has warnings but no errors:

$ yarn check -s
warning "backbone#underscore@>=1.8.3" could be deduped from "1.13.7" to "underscore@1.13.7"
warning Resolution field "ejs@3.1.10" is incompatible with requested version "nightwatch#ejs@3.1.8"
warning Resolution field "nightwatch#semver@7.5.4" is incompatible with requested version "nightwatch#semver@7.3.5"
warning "stylelint#debug@^4.3.7" could be deduped from "4.4.0" to "debug@4.4.0"
warning "eslint#@humanwhocodes/config-array#debug@^4.3.1" could be deduped from "4.4.0" to "debug@4.4.0"

I can further fix the debug and underscore warnings by removing those from the lockfile and running yarn install again. This just leaves:

$ yarn check -s
warning Resolution field "ejs@3.1.10" is incompatible with requested version "nightwatch#ejs@3.1.8"
warning Resolution field "nightwatch#semver@7.5.4" is incompatible with requested version "nightwatch#semver@7.3.5"

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

longwave → created an issue.

longwave → created an issue.

longwave → created an issue. See original summary → .

xjm → credited longwave → .

This is likely to be a compile time issue with your local environment. What does the build.log file referenced in the error say?

Committed 1944dc5 and pushed to 10.4.x. Thanks!

This looks like a nice cleanup, added a couple of minor questions to the MR.

There is a test failure on Drupal 10 only, in the meantime assigning to Wim for review.

Due to holidays there will be no triage on 2024-12-26, moving this ahead two weeks.

Discussed this with @larowlan and @benjifisher. We think the safest option is to move this feature to a submodule, and leave it disabled by default on either new or existing sites. We can mention in the release notes that OEmbed discovery has been moved to a submodule and can be enabled separately, and also as a usability improvement we could suggest installing the module if OEmbed fails in a way that might be because discovery is required.

Is it possible to allow both PHPStan 1 and 2 in composer.json? While core can move to v2 all at once, contrib (including the CI templates) might not want to do so just yet, so allowing a downgrade might be useful.

I think we could add the attributes afterwards, either naively with regex if it's always to a single top level element otherwise we could just do the same DOM parsing and manipulation that Jesse is suggesting, but on the server.

So we are now generating comments server side, and parsing those comments and turning them into data attributes into the client? In the interests of keeping things simple, and with the fact that we control the server side output, why can't we just add those data attributes up front on the server side and save translating anything in the client?

@andypost install both in the container, run Apache by default, but in a specific CI job we kill it and run nginx instead before starting tests?

Now we have GitLab CI, we have the possibility of running some tests on nginx, which would be good to ensure we match the protections we provide for Apache.

MR!471:

• Adds a checkbox to each theme settings page
• When first checked, a page template based on the existing block layout is automatically created

Discussed with the release managers, additionally backported this to 11.1.x and 10.4.x/10.5.x so it can get into the forthcoming releases.

Adding credit for @mcdruid for discussing with me and raising some of the points mentioned in #27.

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?

Landing this one from NR as 10.4.0 is this week and it seems the most practical thing to do to keep this in sync with 11.1.0.

Committed 6b00903 and pushed to 10.5.x. Thanks!

Committed 6995c90 and pushed to 10.4.x. Thanks!