longwave → created an issue. See original summary → .
The problem is that by leaving the method on the interface, even if deprecated, is that anyone else implementing has to also leave the implementation in place. Instead I have still removed the method from the interface, but kept it on the concrete class, and added BC so it still works but issues a deprecation if you call the old method.
This is also too late for 10.3 now, so I have deprecated in 11.1 for removal in 12.0.
There is only one declaration of field_form_field_config_edit_form_entity_builder() in field.module in all current versions of Drupal core, but your error message implies there are two copies - have you patched or manually modified core in some way?
@quietone reminded me this is major version only. 🐛 Skip query string compression if zlib extension isn't available Fixed works around the issue if zlib is not available, but we can still require it to make things simpler in 11.0.
Discussed with @quietone, in order to get the upcoming betas out today we need to commit these today, so committing from NR. Unfortunately we need different patches for each branch, I solved the issues in 11.x and 11.0.x but marking PTBP to get a 10.3.x and 10.4.x patch ready.
Committed 0a4f122 and pushed to 11.x. Thanks!
Committed b3a8376 and pushed to 11.0.x. Thanks!
If we wanted a global switch should this be a permission (or pair of permissions) provided by filter.module?
Also, I've said this in other issues - checking a box to hide something feels the wrong way round, the checkbox should be checked if we are showing something.
Thanks! It's even easier to override than that, you can just add a services.yml alongside your settings.php (and include it from settings.php), and override any container parameter there - updated the IS a bit for this.
Added some questions and suggestions.
The issue title and summary no longer match the changes made in the MR following the discoveries in this issue, they need updating to help reviewers understand the changes that are being made and why.
And we can use the brand new core component for these issues now :D
I know this is all @internal but let's land this now before we cut a release that contains it.
Committed and pushed 2e3e874662 to 11.x and 31ff330aaf to 11.0.x and 43ce451db2 to 10.4.x and 2c3c957208 to 10.3.x. Thanks!
Thanks for volunteering to maintain these new subsystems! I've added the two new subsystems to the component dropdown.
Committed and pushed df213d342f4 to 11.x and c33fdbf2070 to 11.0.x and 7a2eeb7b838 to 10.4.x and 8390744335a to 10.3.x. Thanks!
There were a couple of minor changes to the baseline where the error message has been corrected/improved:
- 'message' => '#^Call to deprecated method getConfig\\(\\) of class GuzzleHttp\\\\ClientInterface\\:
+ 'message' => '#^Call to deprecated method getConfig\\(\\) of interface GuzzleHttp\\\\ClientInterface\\:
This required regenerating the baseline, which now outputs the error identifiers; we should keep this in sync here otherwise we will only have to do it next time we need to regenerate the baseline because we add or remove a PHPStan-reported issue.
Opened 📌 Use consistent fake versions for testing deprecations Active
longwave → created an issue.
Fixed the test, taking the liberty to self-RTBC following #6.
In other tests of the deprecation process itself we use "is deprecated in drupal:X.0.0 and is removed from drupal:Y.0.0", maybe worth a followup to both make this consistent and make it easier to see which ones need to remain forever as they test the system itself.
longwave → created an issue.
+--------------------+--------+---------+
| Production Changes | From | To |
+--------------------+--------+---------+
| psr/http-factory | 1.0.2 | 1.1.0 |
| twig/twig | v3.9.3 | v3.10.2 |
+--------------------+--------+---------+
+------------------------------------+----------+----------+
| Dev Changes | From | To |
+------------------------------------+----------+----------+
| composer/composer | 2.7.2 | 2.7.6 |
| composer/xdebug-handler | 3.0.4 | 3.0.5 |
| mglaman/phpstan-drupal | 1.2.10 | 1.2.11 |
| open-telemetry/sem-conv | 1.24.0 | 1.25.0 |
| phpstan/phpdoc-parser | 1.28.0 | 1.29.0 |
| phpstan/phpstan | 1.10.66 | 1.11.1 |
| phpstan/phpstan-deprecation-rules | 1.1.4 | 1.2.0 |
| phpstan/phpstan-phpunit | 1.3.16 | 1.4.0 |
| sirbrillig/phpcs-variable-analysis | v2.11.17 | v2.11.18 |
| squizlabs/php_codesniffer | 3.9.1 | 3.9.2 |
| webflo/drupal-finder | 1.2.2 | 1.3.0 |
+------------------------------------+----------+----------+
longwave → created an issue.
Actually I guess I should bump the minimum version of PHPStan here.
These are "error identifiers", new in PHPStan 1.11: https://github.com/phpstan/phpstan/releases/tag/1.11.0
This won't make it into 10.3.
+------------------------------------+--------+---------+
| Production Changes | From | To |
+------------------------------------+--------+---------+
| psr/http-factory | 1.0.2 | 1.1.0 |
| symfony/console | v6.4.6 | v6.4.7 |
| symfony/dependency-injection | v6.4.6 | v6.4.7 |
| symfony/deprecation-contracts | v3.4.0 | v3.5.0 |
| symfony/error-handler | v6.4.6 | v6.4.7 |
| symfony/event-dispatcher | v6.4.3 | v6.4.7 |
| symfony/event-dispatcher-contracts | v3.4.2 | v3.5.0 |
| symfony/filesystem | v6.4.6 | v6.4.7 |
| symfony/finder | v6.4.0 | v6.4.7 |
| symfony/http-foundation | v6.4.4 | v6.4.7 |
| symfony/http-kernel | v6.4.6 | v6.4.7 |
| symfony/mailer | v6.4.6 | v6.4.7 |
| symfony/mime | v6.4.6 | v6.4.7 |
| symfony/process | v6.4.4 | v6.4.7 |
| symfony/psr-http-message-bridge | v6.4.6 | v6.4.7 |
| symfony/routing | v6.4.6 | v6.4.7 |
| symfony/serializer | v6.4.6 | v6.4.7 |
| symfony/service-contracts | v3.4.2 | v3.5.0 |
| symfony/string | v6.4.4 | v6.4.7 |
| symfony/translation-contracts | v3.4.2 | v3.5.0 |
| symfony/validator | v6.4.6 | v6.4.7 |
| symfony/var-dumper | v6.4.6 | v6.4.7 |
| symfony/var-exporter | v6.4.6 | v6.4.7 |
| symfony/yaml | v6.4.3 | v6.4.7 |
| twig/twig | v3.9.3 | v3.10.2 |
+------------------------------------+--------+---------+
+-----------------------------------+---------+--------+
| Dev Changes | From | To |
+-----------------------------------+---------+--------+
| composer/composer | 2.7.2 | 2.7.6 |
| composer/xdebug-handler | 3.0.4 | 3.0.5 |
| mglaman/phpstan-drupal | 1.2.10 | 1.2.11 |
| phpstan/phpdoc-parser | 1.28.0 | 1.29.0 |
| phpstan/phpstan | 1.10.67 | 1.11.0 |
| phpstan/phpstan-deprecation-rules | 1.1.4 | 1.2.0 |
| phpstan/phpstan-phpunit | 1.3.16 | 1.4.0 |
| squizlabs/php_codesniffer | 3.9.1 | 3.9.2 |
| symfony/browser-kit | v6.4.3 | v6.4.7 |
| symfony/css-selector | v6.4.3 | v6.4.7 |
| symfony/dom-crawler | v6.4.4 | v6.4.7 |
| symfony/lock | v6.4.6 | v6.4.7 |
| symfony/phpunit-bridge | v6.4.6 | v6.4.7 |
| webflo/drupal-finder | 1.2.2 | 1.3.0 |
+-----------------------------------+---------+--------+
longwave → created an issue.
All green :D
Added CR: https://www.drupal.org/node/3447202 →
Summarised further in the release note snippet in the IS.
Only the build tests failed because I forgot to change minimum stability, so hopefully we are good here.
Yeah this is no longer strictly necessary for 11.0.0 as per #43 we can ideally refactor it away given there is apparently a lot of cruft that supports old browsers nowadays.
Not sure this needs a meta, let's see how these MRs do.
+------------------------------------+--------+--------------+
| Production Changes | From | To |
+------------------------------------+--------+--------------+
| symfony/console | v7.0.6 | v7.1.0-BETA1 |
| symfony/dependency-injection | v7.0.6 | v7.1.0-BETA1 |
| symfony/deprecation-contracts | v3.4.0 | v3.5.0 |
| symfony/error-handler | v7.0.6 | v7.1.0-BETA1 |
| symfony/event-dispatcher | v7.0.3 | v7.1.0-BETA1 |
| symfony/event-dispatcher-contracts | v3.4.2 | v3.5.0 |
| symfony/filesystem | v7.0.6 | v7.1.0-BETA1 |
| symfony/finder | v7.0.0 | v7.1.0-BETA1 |
| symfony/http-foundation | v7.0.6 | v7.1.0-BETA1 |
| symfony/http-kernel | v7.0.6 | v7.1.0-BETA1 |
| symfony/mailer | v7.0.6 | v7.1.0-BETA1 |
| symfony/mime | v7.0.6 | v7.1.0-BETA1 |
| symfony/process | v7.0.4 | v7.1.0-BETA1 |
| symfony/psr-http-message-bridge | v7.0.6 | v7.1.0-BETA1 |
| symfony/routing | v7.0.6 | v7.1.0-BETA1 |
| symfony/serializer | v7.0.6 | v7.1.0-BETA1 |
| symfony/service-contracts | v3.4.2 | v3.5.0 |
| symfony/string | v7.0.4 | v7.0.7 |
| symfony/translation-contracts | v3.4.2 | v3.5.0 |
| symfony/validator | v7.0.6 | v7.1.0-BETA1 |
| symfony/var-dumper | v7.0.6 | v7.1.0-BETA1 |
| symfony/var-exporter | v7.0.6 | v7.0.7 |
| symfony/yaml | v7.0.3 | v7.1.0-BETA1 |
+------------------------------------+--------+--------------+
+----------------------+--------+--------------+
| Dev Changes | From | To |
+----------------------+--------+--------------+
| symfony/browser-kit | v7.0.3 | v7.1.0-BETA1 |
| symfony/css-selector | v7.0.3 | v7.1.0-BETA1 |
| symfony/dom-crawler | v7.0.4 | v7.1.0-BETA1 |
| symfony/lock | v7.0.6 | v7.1.0-BETA1 |
+----------------------+--------+--------------+
xjm → credited longwave → .
longwave → made their first commit to this issue’s fork.
As the practical thing to do for now let's revert this in 10.3/10.4, keep it as-is in 11, and try to revisit the BC layer in 10.4.
Committed and pushed c5ea6561fc to 11.x and cde2856cef to 11.0.x. Thanks!
xjm → credited longwave → .
pameeela → credited longwave → .
Looks fine to me, might as well start chopping away at these where we can.
This is eligible for backport to 10.2.x under the bugfix policy but given it has only arisen in 10.3.x not sure it's worth it. Feel free to reopen for backport if this is incorrect.
Committed and pushed 926d069c2c to 11.x and 55463c9ef0 to 11.0.x and b7c0549a3c to 10.4.x and ef095f9bd2 to 10.3.x. Thanks!
A nice straightforward fix and test. I did wonder what else breaks if you change a machine name - should we have another layer of indirection such as UUIDs that never change? - but this will solve the immediate problem.
Backported to 10.2.x as an eligible bug fix.
Committed and pushed 26e5ef207a to 11.x and 8a7586f81e to 11.0.x and d45f21b776 to 10.4.x and 8367edf312 to 10.3.x and 72e1a9792b to 10.2.x. Thanks!
Yay, this is really nice cleanup, let's land this before the betas.
bbrala → credited longwave → .
longwave → changed the visibility of the branch 3439522-update-javascript-dependencies to hidden.
Bah, we can't update stylelint because of stylelint-formatter-gitlab: https://gitlab.com/leon0399/stylelint-formatter-gitlab/-/issues/3
Actually there aren't that many major changes so we can probably do this all in one issue.
We can't upgrade to ESLint 9 yet as eslint-config-airbnb doesn't support it yet: https://github.com/airbnb/javascript/issues/2961
I've also left out Nightwatch and the pinned deps in 10.3/10.4 but everything else is upgraded to its latest version.
There is some discussion about adding code coverage reports at ✨ Allow run-tests.sh to generate coverage reports Needs work , please read that issue and help out there if you can.
In #3445106-7: Replace @dataProvider annotations with #[DataProvider()] attributes → @mondrake linked to an example where the file has both annotations (for PHPUnit 9) and attributes (for PHPUnit 10/11) at the same time: https://github.com/FileEye/MimeMap/blob/ba0b04e179976e7d6a487fdb166c5f1f...
Comment nit, otherwise LGTM.
Committed and pushed fe3721e8a2 to 11.x and 00317c88e0 to 11.0.x and b7542fbcf4 to 10.4.x and 2bbb3d82f3 to 10.3.x. Thanks!
Committed and pushed 94dde8810e to 11.x and e5099ea862 to 11.0.x and 38a06b5e62 to 10.4.x and 9f0f63eafc to 10.3.x. Thanks!
Simplified this even further, if we redirect at all then we shouldn't show the message?
Debating whether we should have a test that validates all recipes in core before we even commit them, but perhaps that is a good followup.
Rebased onto 11.x.
longwave → made their first commit to this issue’s fork.
This does seem like it would be useful but should this replace the Symfony constraint or just be a new Drupal specific one that extends Choice?
Thanks for working on this! There are two branches but no open merge requests; please open a merge request on the branch that has the correct changes, and hide the other branch so as not to confuse reviewers.
Setting back to fixed as #133/134 was solved in 🐛 Composer tests fail because of invalid Drupal version Fixed
Looks good, can't find any other references to this script.
longwave → created an issue.
longwave → changed the visibility of the branch 3445211-fix-drupal-version-constant to hidden.
Not entirely sure why this has started failing now, but the root cause is that drupal-11.0-dev.es.po ends up as a local translation which was then failed to be parsed by _install_get_version_info() because there is no patch component in the version number.
Experiments are ongoing over in 🐛 Composer tests fail because of invalid Drupal version Fixed , we don't think it is directly PHPUnit 10 - it's (partially?) something to do with translation downloads, but I don't think this should entirely be rolled back so marking as postponed on that issue for now.
@catch this reminds me of
#3225466: Blank line in .module file causes blank line in HTML output →
which I debugged by adding code to ModuleHandler::loadInclude to detect which file caused the issue - I wonder if that should be a standard core feature.
longwave → made their first commit to this issue’s fork.
#15 strikes me as being the same root cause as 🐛 Adding media library openers use autoconfigure and tags in 10.3.x has BC consequences Active so I think worth seeing if we can solve both problems with one solution.
There's a few stragglers:
core/scripts/drupal.sh
16:trigger_error('drupal.sh is deprecated in drupal:10.1.0 and is removed from drupal:11.0.0. There is no replacement. See https://www.drupal.org/node/3241346', E_USER_DEPRECATED);
core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
105: @trigger_error(sprintf('%s called unnecessarily in a test is deprecated in drupal:10.2.0 and will throw an exception in drupal:11.0.0. See https://www.drupal.org/node/3401201', __METHOD__), E_USER_DEPRECATED);
core/tests/Drupal/FunctionalJavascriptTests/WebDriverTestBase.php
146: @trigger_error('The "chromeOptions" array key is deprecated in drupal:10.3.0 and is removed from drupal:11.0.0. Use "goog:chromeOptions instead. See https://www.drupal.org/node/3422624', E_USER_DEPRECATED);
core/tests/Drupal/Tests/BrowserTestBase.php
457: @trigger_error('Pushing requests without a session onto the request_stack is deprecated in drupal:10.3.0 and an error will be thrown from drupal:11.0.0. See https://www.drupal.org/node/3337193', E_USER_DEPRECATED);
core/tests/Drupal/Tests/UnitTestCase.php
72: @trigger_error('Accessing the randomGenerator property is deprecated in drupal:10.2.0 and is removed from drupal:11.0.0. Use getRandomGenerator() instead. See https://www.drupal.org/node/3358445', E_USER_DEPRECATED);
I don't understand what has changed here, the chdir() was added a while back in
🐛
InfoParser returns an empty array if passed a non-existing file
Fixed
so why is PHPUnit 9 behaviour different?
I think the MR will work but the services are meant to be destructible for a reason, maybe we should issue a warning/deprecation when we find a non DestructableInterface service so downstream users are notified?
longwave → made their first commit to this issue’s fork.
Committed and pushed 26208e91c0 to 11.x and 079b65a232 to 11.0.x and 517e75283e to 10.4.x and 77d19f7abb to 10.3.x. Thanks!
Committed and pushed 32b8344b7f to 11.x and 1213712a62 to 11.0.x. Thanks!
As spotted over in #3417066-120: Upgrade PHPUnit to 10, drop Symfony PHPUnit-bridge dependency → , it looks like we can't mix PHPUnit attributes and annotations, we will have to convert them all at once unfortunately:
$metadata = $this->attributeReader->forMethod($className, $methodName);
if (!$metadata->isEmpty()) {
return $metadata;
}
return $this->annotationReader->forMethod($className, $methodName);
quietone → credited longwave → .
Given the API change documented in 🐛 Install profile is disabled for lots of different reasons and core doesn't allow for that Fixed the fix here is obvious so I also think this can go in without tests.
Summarised #122-#124 in the change record.
Removed the process isolation section from the CR, as that is obsolete now.
FWIW for core development I don't use a custom phpunit.xml, instead I override various environment variables in ddev, which works the same on both versions of PHPUnit.
Thanks to everyone who contributed here, and congratulations on landing this, can't wait to see what we can do in the future now this is available in core.
Committed 74da82a and pushed to 11.x. Thanks!
Committed and pushed 54be1ad9d2 to 10.4.x and a15d5540c6 to 10.3.x. Thanks!
Addressed #120, as it's only undoing attributes added while we were working on the process isolation fixes I think this is OK - my RTBC covers @mondrake's changes since @larowlan previously marked this RTBC.
We can remove the attributes added for testing again here, but after that I think this is good to go.
Also it looks like we can't mix PHPUnit attributes and annotations, we will have to convert them all at once unfortunately:
$metadata = $this->attributeReader->forMethod($className, $methodName);
if (!$metadata->isEmpty()) {
return $metadata;
}
return $this->annotationReader->forMethod($className, $methodName);
Thanks! I realise what was happening before now:
When run-tests.sh was configuring process isolation, FunctionalTestDebugHtmlOutputTest runs FunctionalTestDebugHtmlOutputHelperTest separately - without isolation - so the trait works and it passes.
When isolation was configured in the constructor, it affects all tests, including FunctionalTestDebugHtmlOutputHelperTest, so the trait stopped working.
I am not sure we should add the attribute to all tests, because it means remembering to add it each time (and probably writing a test or PHPStan rule to enforce it) in core, contrib and custom tests. Surely it's better if we just configure this automatically, just like we did in PHPUnit 9 and below?
I still don't understand how that one acts differently from the constructor change here, not how they then again act differently when run-tests.sh sets the process isolation flag.
The issue is that the events fire in the parent process, and the logging happens in the child process. When process isolation is used the static $links never gets populated in the parent, and so the output is never printed. We need some way of communicating back from the child that output was printed - maybe we can use the counter file for this instead of a static variable?
I confirmed with xdebug that the environment variables are passed into the child process, and the HtmlOutputLogger appears to work, except that HtmlOutputLogger::testRunnerFinished() appears to not be called inside the isolated process, and so the verbose message is not printed. Will continue debugging...
Thanks @mondrake - I see the error I made last night now, I forgot to escape the () in the deprecation.