Second time lucky. Backported to 11.1.x as this is a test-only fix.
Committed and pushed 2d31d3e8f14 to 11.x and 1c881a1efb5 to 11.2.x and 60d98a67e99 to 11.1.x. Thanks!
Crediting @larowlan as this came from a comment by @larowlan.
I think we should retitle this issue. It's not really about test classes anymore. Plus I left quite a bit of feedback on the MR.
alexpott → changed the visibility of the branch 11.x to active.
This has been solved in Drupal 11 - see ✨ Allow explicit COMMIT in Transaction objects Active - contributors to this issue were credited on that issue.
@jose reyero I created the merge request for you - https://git.drupalcode.org/project/drupal/-/merge_requests/12121
FWIW does this actually happen on 10.5.x? I think state cache collecting only was turned on by default in 11.x. - anyhows no harm in the tests being aligned.
Committed 7aa2683 and pushed to 10.5.x. Thanks!
Getting this into 11.x early in the 11.3.x cycle to wrangle out any issues and work on the follow-ups.
Committed 4815dcc and pushed to 11.x. Thanks!
Crediting people who contributed to 📌 Transactions should be allowed to be committed explicitly Needs work - ie. c960657 and anybody.
Please all the peeps from this issue.
The revision log message formatter should allow the same list of tags are we do in \Drupal\node\Controller\NodeController::revisionOverview()
$column = [
'data' => [
'#type' => 'inline_template',
'#template' => '{% trans %}{{ date }} by {{ username }}{% endtrans %}{% if message %}<p class="revision-log">{{ message }}</p>{% endif %}',
'#context' => [
'date' => $link,
'username' => $this->renderer->renderInIsolation($username),
'message' => ['#markup' => $revision->revision_log->value, '#allowed_tags' => Xss::getHtmlTagList()],
],
],
];
In this case we need to change the formatter for the revision log message to be able to use basic html. Then we can use it in views. If you look at the revision page after doing a revert the log message is perfectly rendered em tags and all.
I think we should fix the table to allow html. It means all the existing messages will work.
As a minor feature request committed to 11.2.x so it will be part of 11.2.0 even though we are in alpha. This is very low disruption.
Committed and pushed ed28d46cb09 to 11.x and 176346e770b to 11.2.x. Thanks!
Backported to 11.1.x as a test-only change.
alexpott → created an issue.
Backported to 11.1.x as a bug fix.
Committed and pushed 420f7b2ff2d to 11.x and 180597712ad to 11.2.x and a94e3c35cb7 to 11.1.x. Thanks!
Backported to 11.1.x as a bugfix.
Committed and pushed 42fb139e294 to 11.x and bd08e821d86 to 11.2.x and 335bc707498 to 11.1.x. Thanks!
Backported to 11.1.x as test-only change.
Committed and pushed c8a7a5894c9 to 11.x and 5f901886979 to 11.2.x and bc806750d89 to 11.1.x. Thanks!
I wish we could just make PHPUnit responsible for this and remove #slow completely. It should just run the slowest tests from the last run first...
Committed and pushed bccd5a0c920 to 11.x and f2a221b3523 to 11.2.x and 00080f5aa17 to 11.1.x. Thanks!
Backported to 11.1.x as a test only change.
There are 282 test methods in grep -R "public function test" core/tests/Drupal/KernelTests/Core/Entity | wc -l before and after applying the patch and the lest change was 4 weeks ago which is before #7 so I think we are good here.
The only way to fix the baseline stuff is to add return typehints to all the test traits - which would be a great thing to do imo.
Committed and pushed 6d6f909d46e to 11.x and 699effc461f to 11.2.x. Thanks!
Fair enough. We're not losing test coverage here.
Committed and pushed 6b577ab50be to 11.x and fb605312a2e to 11.2.x. Thanks!
Committed and pushed b8c04763730 to 11.x and cf5008b15b1 to 11.2.x and d9535798624 to 11.1.x. Thanks!
Backported to 11.1.x as this is a test-only change and makes it easier to backport things.
Committed and pushed 2999d4574c4 to 11.x and 3df1b8d2897 to 11.2.x. Thanks!
Committed and pushed 656836718c4 to 11.x and 4eb755b42fc to 11.2.x. Thanks!
Can be set to RTBC when done and I will prioritise.
Given #17 we should add a release note snippet for 11.2.0 - probably pointing to https://github.com/jsonrainbow/json-schema/blob/master/UPGRADE-6.0.md to be helpful.
Committed and pushed 7d1a9ca93b3 to 11.x and a66d9a73577 to 11.2.x and 9a34ed55ded to 11.1.x. Thanks!
The issue summary is out-of-date with the final state of the patch. It could do with being updated to match. The proposed resolution and other sections are all out-of-date.
The MR looks good. Once the issue summary has been updated can be set back to RTBC and I will prioritise this one.
I'm not sure about this one. Drupal is really special in how we futz with the autoloader to make test traits autoloadable (and in fact all the tests). This is not normal behaviour.
The issue summary needs to be updated with a concrete use case of why this is or might be a good idea. Atm the issue summary is just about why you can't do it not why to do it.
Perhaps more elegant:
// Remove any third party settings for modules that are not installed.
$theme_config = $this->configFactory->getEditable($key . '.settings');
if (!$theme_config->isNew()) {
$third_party_settings = array_filter(
$theme_config->get('third_party_settings') ?? [],
fn ($module) => !$this->moduleHandler->moduleExists($module),
ARRAY_FILTER_USE_KEY
);
if ($third_party_settings !== $theme_config->get('third_party_settings') ?? []) {
$theme_config->set('third_party_settings', $third_party_settings)->save();
}
}
This is going to change what is set up when you install olivero with shortcuts already enabled. This must change what happens when you install standard.
I think we need to go a little further and introduce a little bit of cleverness into the theme installer. Something along the lines of:
diff --git a/core/lib/Drupal/Core/Extension/ThemeInstaller.php b/core/lib/Drupal/Core/Extension/ThemeInstaller.php
index 364d672c12f..c99f6a07a30 100644
--- a/core/lib/Drupal/Core/Extension/ThemeInstaller.php
+++ b/core/lib/Drupal/Core/Extension/ThemeInstaller.php
@@ -240,6 +240,21 @@ public function install(array $theme_list, $install_dependencies = TRUE) {
if (!isset($installed_themes[$key])) {
// Install default configuration of the theme.
$this->configInstaller->installDefaultConfig('theme', $key);
+
+ // Remove any third party settings for modules that are not installed.
+ $theme_config = $this->configFactory->getEditable($key . '.settings');
+ if (!$theme_config->isNew()) {
+ $theme_config_changed = FALSE;
+ foreach ($theme_config->get('third_party_settings') ?? [] as $module => $third_party_settings) {
+ if (!$this->moduleHandler->moduleExists($module)) {
+ $theme_config->clear("third_party_settings.$module");
+ $theme_config_changed = TRUE;
+ }
+ }
+ if (!$theme_config_changed) {
+ $theme_config->save();
+ }
+ }
}
$themes_installed[] = $key;
Discussed with @catch we agreed that this can go in the 11.2.x alpha period.
Committed and pushed 5301cfd2371 to 11.x and 94ccff04de2 to 11.2.x. Thanks!
I guess we could change trigger_error('A language_switch_links hook implementation has changed $result to a non-array.', E_USER_WARNING); to a proper deprecation and say that in Drupal 12 this will cause an exception when we remove the check...
Here's how I think the code should look:
public function getLanguageSwitchLinks($type, Url $url) {
if ($this->negotiator) {
foreach ($this->negotiator->getNegotiationMethods($type) as $method_id => $method) {
if (is_subclass_of($method['class'], LanguageSwitcherInterface::class)) {
$original_languages = $this->negotiatedLanguages;
try {
$result = $this->negotiator->getNegotiationMethodInstance($method_id)
->getLanguageSwitchLinks($this->requestStack->getCurrentRequest(), $type, $url);
if (!empty($result)) {
// Allow modules to provide translations for specific links.
$this->moduleHandler->alter('language_switch_links', $result, $type, $url);
if (!is_array($result)) {
trigger_error('A language_switch_links hook implementation has changed $result to a non-array.', E_USER_WARNING);
$result = [];
}
$result = array_filter($result, function (array $link): bool {
$url = $link['url'] ?? NULL;
$language = $link['language'] ?? NULL;
if ($language instanceof LanguageInterface) {
$this->negotiatedLanguages[LanguageInterface::TYPE_CONTENT] = $language;
$this->negotiatedLanguages[LanguageInterface::TYPE_INTERFACE] = $language;
}
try {
return $url instanceof Url && $url->access();
} catch (\Exception) {
return FALSE;
}
});
if (empty($result)) {
return NULL;
}
$links = (object) ['links' => $result, 'method_id' => $method_id];
break;
}
}
finally {
$this->negotiatedLanguages = $original_languages;
}
}
}
}
return $links ?? NULL;
}
We should never be doing $links = $config->get('hide_single_link_block') ? NULL : []; - $link must remain an array otherwise this module is not interoperable with other modules that use the hook.
We need a core issue that does not return an object when $result is empty in \Drupal\language\ConfigurableLanguageManager::getLanguageSwitchLinks() - we can do that in 🐛 LanguageManager does not check against altered language_switch_links Needs review
I think we should be logging an error when $results is altered to a non array type. Even though that used to work before the security issue fix it is non interoperable with other hook implementation.
Also I think we should change the code to obey the interface and return NULL if the array filter results in $results being empty.
Furthermore I think we need a try {} finally {} here to ensure that $this->negotiatedLanguages is reset regardless of what we do.
Committed and pushed bc761e4a9d4 to 11.x and ec3617b375e to 11.2.x and e9470a68e61 to 11.1.x. Thanks!
Backported to 11.1.x too because that makes sense to me.
Committed and pushed 905a8ddd202 to 11.1.x and d1e3c80b3bf to 10.5.x. Thanks!
Missing docs...
I keep on pondering whether we should decouple the config removal from the other changes here. Like decouple views RSS and make the system config completely unused and deprecated and then remove that in a separate 12.x issue. I would mean that we also have the previous value around for module updates.
Not sure - going to ping @catch and @longwave for thoughts.
Committed and pushed b43aae5dd67 to 11.x and 88dc606eba6 to 11.2.x and 8c347d75fe6 to 11.1.x. Thanks!
@jose reyero #13 didn't have the updated patch. Can we use the MR workflow? See the issue fork stuff at the bottom of the issue summary. Thanks!
I think explicit options that match the arguments of the object is definitely the way to go. Really like the way this is going. Lovely work.
Committed and pushed e298fb7bb23 to 11.x and f9572f33db2 to 11.2.x. Thanks!
Adding issue credit.
Yes this is still an issue. And yes we should convert the warning from the docs to something we're warning about in the system.
In fact the whole way the global metatag object is loading and then changed using \Drupal\metatag\Entity\MetatagDefaults::overwriteTags is quite interesting. This will end up changing the cached config object in memory (if we turned on memory caching for metatag defaults config entities).
I think we should try to tidy all of this up and allow the metatags system to work when the global defaults are disabled.
alexpott → created an issue.
Search link that found the modules that need fixes... http://codcontrib.hank.vps-private.net/search?text=system.rss&filename=
We have two modules that will need issues against when we remove system.rss. https://www.drupal.org/project/knowledge → and https://www.drupal.org/project/support → - the Knowledge project is the only one with a stable release against 11.x ... some other modules will require updating but it is
The change record needs to be updated to explicit mention that system.rss is going away and what modules should do to replace it. Setting to needs work to address this. Once done can be rtbc'd again.
@himani_219 can you link the Composer github issue - thanks! I've looked in the recently update Composer issues and can not find it.
Given this was set to needs work for a reroll - setting back to RTBC as the reroll is done.
alexpott → created an issue.
The updates made by @grimreaper look great.
hestenet → credited alexpott → .
hestenet → credited alexpott → .
hestenet → credited alexpott → .
alexpott → created an issue.
I think we should consider improving the architecture rather than injecting yet another service into the theme installer. I think we could even remove the clear cache method if we listen to configuration events.
@bbrala great work on looking! I was thinking of doing the same - so nice to see it done.
In fact the dynamic file generation is problematic because it is never cleaned up. Let's just not do that.
Borrowed code from build tests.
alexpott → created an issue.
Just pushed some changes that will fix #98... pretty sure... added test coverage after reproducing the issue with a minimal set.
FWIW this MR is ready for extensive testing. I have pinged the Symfony and Composer maintainers to see if I can get some feedback on the gnarlier bits of futzing with Composer internals as I'm not sure how easy it is to review this part of the MR.
Added a change record.
I'm not sure why we need to add test coverage of the package manager here. I think the package manager needs to obey composer plugins or we have a problem.
There's a little bit more we need to cope with.
We need to test what happens when the profile is NULL - that’s the starting state. And when it is complete removed - that’s what happens when the install profile is uninstalled.
In the case that the profile is not set I think we'll need to set the directories to something that cannot exist - like we do in \Drupal\Core\Extension\InstallProfileUninstallValidator::getExtensionDiscovery
Also I think that \Drupal\Core\Extension\Plugin\Validation\Constraint\ExtensionAvailableConstraintValidator::$extensionDiscovery should be an array of extension discoveries keyed by profile as this is easy to reason about. !in_array($profile, $this->extensionDiscovery->getProfileDirectories()) looks interesting and fragile.
The current approach is not going to work unfortunately. Because the ExtensionList objects interact with state and cache so we're in danger of affected the live running of a site if we validate a core.extension object prior to it being changed. I think we have to follow InstallProfileUninstallValidator's example and use a lower level ExtensionDiscovery object to determine if modules and themes exist if the profile has changed.
Tested the fix on a site - works great.
alexpott → created an issue.
Re
"I'm not sure. I guess we can leave it alone until it causes us problems."
What I mean is we can ignore trying to validate profile beyond a profiles existence for now. I think we should not be changing that test in this issue. The fact we are shows we've got stuff to fix.
Re profile changes - we already only really allow the profile to be set by the installer and unset though module uninstallation after checking the modules - see \Drupal\Core\Extension\InstallProfileUninstallValidator. In any real way we don't actually support making changes to core.extension directly via config tools. So the test that I've commented on is pretty funky. I do think that 1 is probably the best solution for module and theme validation. I.e if the profile has changed use a fresh discovery object. Wrt. to enforcing install profile change rules... I'm not sure. I guess we can leave it alone until it causes us problems.
Changing the install profile on an existing site is a little fraught. Things have change so that you can uninstall an install profile if you are not using any themes or modules that only exist in the install profile - see \Drupal\Core\Extension\InstallProfileUninstallValidator.
We could consider changing an install profile to another install profile to be an invalid change and only allow it to be cleared. Hmmm. Not sure.
We need to address the comment I've just added to the MR... there's a very very awkward thing with profiles. In fact the comment does not quite cover the complexity here. I think we need to detect if the profile is changing and if it is we need to rebuild the module and theme validators as this alters what modules and themes can be discovered.
@jose reyero awesome to see you in this issue! And great idea.
Given we're only using Symfony's YAML parser now the idea of using tags is viable. The thing that jumps out is what about translation context? I don't think there is a way to provide an argument to a tag unless we agree some funky character with which to split the string... wouldn't be the first time we've used such an approach in translation. Also I really wish that translation did a fallback approach to context. If it did we could add the action ID as a context to all strings and be done.
We have a problem...
If you have a composer.json as follows
{
"name": "test/test",
"type": "project",
"require": {
"drupal/core-recipe-unpack": "@dev",
"composer/installers": "^2.3"
},
"repositories": {
"composer-unpack": {
"type": "path",
"url": "/PATH/TO/CORE/CHECKOUT/WITH/MR/composer/Plugin/RecipeUnpack",
"options": {
"symlink": true
}
},
"drupal": {
"type": "composer",
"url": "https://packages.drupal.org/8"
}
},
"extra": {
"installer-paths": {
"core": ["type:drupal-core"],
"libraries/{$name}": ["type:drupal-library"],
"modules/contrib/{$name}": ["type:drupal-module"],
"profiles/contrib/{$name}": ["type:drupal-profile"],
"themes/contrib/{$name}": ["type:drupal-theme"],
"drush/Commands/contrib/{$name}": ["type:drupal-drush"],
"modules/custom/{$name}": ["type:drupal-custom-module"],
"themes/custom/{$name}": ["type:drupal-custom-theme"]
}
},
"minimum-stability": "dev",
"prefer-stable": true,
"config": {
"allow-plugins": {
"drupal/core-recipe-unpack": true,
"composer/installers": true
}
}
}
and then do the following:
1. composer install
2. composer require drupal/events
3. composer require drupal/jwt
Step 2 will successfully unpack the recipe but when you do Step 3 it'll detect the recipe has been removed and uninstall it. Which is something we want to avoid...
Oh dear.
I've addressed #89 but in doing that I've revealed the two of the tests are resulting in invalid composer files so we need to fix that.
Oh and I think we should add a follow-up to implement the ability to unpack multiple or even all the recipes in a root composer.json. Atm the unpack command accepts a single package name.
Okay I think we're 99% done. We just need to add tests for two things:
- The composer lock hash is correct after unpacking a recipe.
- The recipe files are left alone after unpacking.