longwave → changed the visibility of the branch 2066993-magic-deprecation to hidden.
Added a comment but I think we could land this as-is and open some followups for deprecations and refactoring?
When thinking about 🐛 [random test failure] Drupal\Tests\system\Functional\System\ThemeTest::testSwitchDefaultTheme() Active before reading this I had a similar idea to the "magic" branch proposed here, nice to see that it works.
This might mean we can remove one or both of these lines in BrowserTestBase::installDrupal():
// Clear the static cache so that subsequent cache invalidations will work
// as expected.
$this->container->get('cache_tags.invalidator')->resetChecksums();
// Explicitly call register() again on the container registered in \Drupal.
// @todo This should already be called through
// DrupalKernel::prepareLegacyRequest() -> DrupalKernel::boot() but that
// appears to be calling a different container.
$this->container->get('stream_wrapper_manager')->register();
Posted that after walking away for a bit - I see what you mean:
$this->adminUser = User::create([
'name' => $this->randomMachineName(),
'roles' => [$admin_role->id()],
]);
$this->adminUser->save();
...
$comment = Comment::create([
'subject' => 'My comment title',
'uid' => $this->adminUser->id(),
'name' => $this->adminUser->label(),
'entity_type' => 'entity_test',
'field_name' => 'comment',
'entity_id' => $host->id(),
'comment_type' => 'entity_test_comment',
'status' => 1,
]);
$comment->save();
We use the adminUser uid so it shouldn't matter.
Fair enough, given this is a kernel test let's just solve this here and move on.
Does that point to an underlying bug somewhere else though? I assume it's related to uid 0 being anonymous but why does Postgres not want to create a uid 1?
longwave → changed the visibility of the branch 3351615-node18 to hidden.
longwave → made their first commit to this issue’s fork.
This might be why Nightwatch and friends failed, the new version of JSDom needs Node v20 according to the docs.
Committed and pushed a30c38e4213 to 11.x and c50910413f0 to 11.3.x. Thanks!
Didn't mean to change this to NW. Also tested in PHPStorm and this works well.
One reason that PHPStan doesn't see any difference is that we aren't calling \Drupal::service() with a class string very often - for historical reasons most tests will use the custom service name, and this change doesn't help with that.
I suspect we should be being more precise where possible, especially if we know the request was a GET and the data will always be in the querystring for example. Symfony will likely have done it for good reason, we should dig up the Symfony issue/PR where this was changed and find their reasoning for doing it which might help us.
The related issue landed so this should be doable now.
Thanks to the autowiring work, given we can already do
> \Drupal::service(\Drupal\Core\Entity\EntityTypeManagerInterface::class);
= Drupal\Core\Entity\EntityTypeManager {#4185}
is there something clever we can do with the docblock of \Drupal::service() so PHPStan can infer the type if you pass an interface name to the existing method, without having to add some new API?
Worked around that with an option to postcss-preset-env:
'break-properties': false,
There are still 23 stylelint fails, all for Unexpected deprecated property "clip" - apparently this needs converting to clip-path but I don't yet understand how to make this change.
Not sure what to do here as PostCSS and stylelint are conflicting.
PostCSS changes
break-inside: avoid;
to
page-break-inside: avoid;
break-inside: avoid;
but Stylelint flags page-break-inside as deprecated and wants to remove it again.
Works for me locally if I run the test alone at least, so not sure what the issue is. Maybe we just roll this back?
$ yarn test:nightwatch modules/ckeditor5/tests/src/Nightwatch/Tests/drupalHtmlBuilderTest.js
[dotenv@17.2.3] injecting env (0) from .env -- tip: 🔑 add access controls to secrets: https://dotenvx.com/ops
[Tests/drupalHtmlBuilderTest]
✔ should return empty string when empty DocumentFragment is passed
✔ should create text from single text node
✔ should return correct HTML from fragment with paragraph
✔ should return correct HTML from fragment with multiple child nodes
✔ should return correct HTML scripts and styles
✔ should return correct HTML from fragment with comment
✔ should return correct HTML from fragment with attributes
✔ should return correct HTML from fragment with self closing tag
✔ attribute values should be escaped
Yeah it's not worth the effort of doing this to save about six lines of code.
Let's leave stylelint-config-standard for another issue, as that involves a bunch of manual CSS changes.
Thanks for starting this. Upgraded everything else here, this just leaves CSpell to go to v9 and ESLint also to go to v9.
You can use the default_value plugin to handle the case where the source value is NULL, so perhaps we just remove this from the static_map docs?
As discussed in #10/11 not sure we should do that here, maybe let's just fix up the docs and then bulk-fix the properties later.
Well, exactly - that's probably worth doing in bulk in a followup.
Discussed briefly with @mondrake, I think it would be much better if we used typed properties:
/**
* The current user logged in using the Mink controlled browser.
*/
protected UserInterface|false $loggedInUser = FALSE;
or even better if we could use
/**
* The current user logged in using the Mink controlled browser.
*/
protected ?UserInterface $loggedInUser;
Thanks for working on this - looks like this has to wait until 📌 Update to Symfony 7.4.0-BETA1 Active is committed.
Thanks. Not sure what was going on with the performance tests earlier, but it seems only the StylesheetBytes needs tweaking.
@andypost Followups for the deprecations all exist as children of 📌 Update to Symfony 7.4.0 Postponed
Not sure we need a change record, we haven't made any API changes - this is just a release notes thing really. We might need change records for some of the deprecations but that will happen in the followups.
Would it be better to use null instead of false? There might be some BC concerns doing that though.
Bumped to BETA2: https://symfony.com/blog/symfony-7-4-0-beta2-released
We will have to figure out how to solve the ampersand issue over in 📌 Since symfony/dom-crawler 7.4: Disabling HTML5 parsing is deprecated. Symfony 8 will unconditionally use the native HTML5 parser Active . The code generates some very questionable HTML for the sake of an XSS-like test but I'm not sure how valid it really is.
This has shaved 4kb (about 9%) off the CSS byte count for the navigation performance test.
Since December 2023, CSS can use the & selector for nesting. This should be fine for all browsers we support:
https://developer.mozilla.org/en-US/docs/Web/CSS/Nesting_selector#browse...
This means the built CSS is easier to read, but has a lot of changes.
Might have to do this with multiple child issues as there's quite a few different pieces to review.
longwave → made their first commit to this issue’s fork.
Symfony 7.4 has chosen to default to the PHP HTML5 parser in their DomCrawler component if you are on PHP 8.4, and it will become the only option in Symfony 8, so we should probably think about doing the same.
This is now the minimal amount of changes required to land 7.4.0-BETA1, all deprecation skips or other temporary fixes (such as the "UTF-8" to "utf-8" change) have been moved to child issues of 📌 Update to Symfony 7.4.0 Postponed .
Opened 📌 Update to Symfony 7.4.0 Postponed with several child issues to fix the deprecations introduced in Symfony 7.4.0-BETA1.
First green run. Let's try adding back symfony/validator...
Trying to make the MR as minimal as possible, we will have to deal with new deprecations in followups (but ideally the big ones need to be removed before 11.3.0 so contrib gets notified of them)
Currently has the following changes:
- Symfony 7.4.0-BETA1 (except symfony/validator, which has even more deprecations)
- Four new deprecation skips, which will each need a followup to solve
- Change of the DomCrawler used by BrowserTestBase to use the non-HTML5 parser in PHP 8.4 (there are some incompatibilities that I have opened Symfony issues for)
- Change of the default Content-Type header charset from "UTF-8" to "utf-8" (affects many tests)
longwave → changed the visibility of the branch 3554533-symfony-7.4 to hidden.
The second regression is fixed in Symfony 7.4.x-dev, but I found a third: https://github.com/symfony/symfony/issues/62236
I think this is a second regression in Symfony: https://github.com/symfony/symfony/issues/62233
Some fun here in that DomCrawler now behaves differently on PHP 8.3 and 8.4 - on 8.4, Symfony now uses the native HTML5 parser instead of masterminds/html5 - so we need to make sure we run tests on both.
See https://github.com/symfony/symfony/commit/bd7fb5178b998e4f863d388850e93b...
I think this is the cause of some/most of the remaining test failures.
There are a bunch of functional test failures that I can't reproduce locally, and some Ajax JavaScript failures that I haven't had chance to investigate yet.
Found a regression in Symfony: https://github.com/symfony/symfony/issues/62226
Trying an upgrade of everything except symfony/validator, which hopefully we can deal with in a followup given there are many new deprecations.
I am not sure we need to do this given this happens immediately prior to instantiation; in recent discussions I think most people have agreed that the expensive part of reflection is loading the class, but if we are loading the class anyway then it makes little difference.
Perhaps worth postponing on 📌 Allow plugin service wiring via constructor parameter attributes Needs work as we should come to the same conclusion in both issues.
Thanks for your help Aaron, I am sure I will see you around in other issues or in person again at DrupalCon or a camp some time soon!
> there's code to convert $options not passed as an array to an array with a value key
I think in a followup we should deprecate this behaviour, and enforce that an array must be passed, given that in D12/SF8 we will only be able to use named arguments anyway. I am not sure value is a sensible default here anyway, it should use the result of getDefaultOption() surely?
For me we don't need additional tests here as this is all necessary work to get the deprecation skip removed. The followups might need some when we start issuing our own deprecations as there will be multiple subtly different code paths there.
Bumping to critical as this is blocking SF7.4.
@godotislate not too sure it matters given we are changing the namespaces anyway, can't remember what the break was between annotations 1 and 2. Also not sure what to do about the exception BC layer though as that isn't supported in the same way in D10.
Discussed with @nicxvan and @catch in Slack. To provide BC I thought I had a way to add a temporary service with !service_locator that could then be pulled from the container, but without also adding a temporary proxy/wrapper class I don't actually think this is possible. Given that extending ThemeManager seems unlikely we decided to go ahead here without the BC layer.
Discussed with @justafish and @nod_ who have no strong preference and have not used Biome, but suggested that if we stay with ESLint 9 then we could just use the standard style instead of the airbnb rules.
As that seems to be the simplest move forward here then let's try that to start with, if it turns out standard isn't enough for us for some reason then we could consider airbnb-extended.
Started looking at tests, but wondering if we should split this out into more issues. If we can remove the deprecation skip here and do the minimum amount of work in ConstraintFactory - perhaps not even issue the deprecation - then that unblocks the start of the Symfony 7.4 upgrade. Then in a followup we try to upgrade to symfony/validator 7.4, fix the new deprecations from there, and start issuing our own deprecations?
I also wonder if we want to do something similar to ConstraintManager's $options argument in a followup, or if we shouldn't bother.
Made a start on a change record: https://www.drupal.org/node/3554746 →
Finding it impossible to debug most of the remaining failures due to way too many deprecations via 📌 Passing a $options array to constraint constructors is deprecated, use named arguments instead Postponed
I looked at git history for the two test methods removed here, they date back to the original implementation of our own container. At the time I guess they were added to ensure we had coverage of anything you might want to pass in, now we have more type safety by providing parameter types we don't need them at all.
Rebased, but added some self review comments; not sure what to do about BC here.
LGTM. Argument types are safe to add as long as they are already on the parent class/interface, this doesn't break compatibility (unlike return types).
Run into this again via 📌 Update to Symfony 7.4.0-BETA1 Active
I wonder if we need to worry about issuing our own deprecation here, or can we just rely on the Symfony one if we fix up our own cases first? Can we just do some check in the factory whether we call the constructor with a single array or the splat operator?
This is where we run into
📌
Passing a $options array to constraint constructors is deprecated, use named arguments instead
Postponed
, plus we can't call $request->get() directly any more, we have to specifically know whether we want POST (request) or GET (query) data; but in Drupal we sometimes use both interchangeable...
In fact let's update Symfony elsewhere as there are a bunch of new deprecations and it looks like they won't be entirely trivial.
Reverted the last commit, back to RTBC.
https://symfony.com/blog/symfony-7-4-0-beta1-released
Updated all Symfony components:
+---------------------------------+--------+--------------+
| Production Changes | From | To |
+---------------------------------+--------+--------------+
| symfony/console | v7.3.4 | v7.4.0-BETA1 |
| symfony/dependency-injection | v7.3.4 | v7.4.0-BETA1 |
| symfony/error-handler | v7.3.4 | v7.4.0-BETA1 |
| symfony/event-dispatcher | v7.3.3 | v7.4.0-BETA1 |
| symfony/filesystem | v7.3.2 | v7.4.0-BETA1 |
| symfony/finder | v7.3.2 | v7.4.0-BETA1 |
| symfony/http-foundation | v7.3.4 | v7.4.0-BETA1 |
| symfony/http-kernel | v7.3.4 | v7.4.0-BETA1 |
| symfony/mailer | v7.3.4 | v7.4.0-BETA1 |
| symfony/mime | v7.3.4 | v7.4.0-BETA1 |
| symfony/process | v7.3.4 | v7.4.0-BETA1 |
| symfony/psr-http-message-bridge | v7.3.0 | v7.4.0-BETA1 |
| symfony/routing | v7.3.4 | v7.4.0-BETA1 |
| symfony/serializer | v7.3.4 | v7.4.0-BETA1 |
| symfony/validator | v7.3.4 | v7.4.0-BETA1 |
| symfony/var-dumper | v7.3.4 | v7.4.0-BETA1 |
| symfony/yaml | v7.3.3 | v7.4.0-BETA1 |
+---------------------------------+--------+--------------+
+----------------------+--------+--------------+
| Dev Changes | From | To |
+----------------------+--------+--------------+
| symfony/browser-kit | v7.3.2 | v7.4.0-BETA1 |
| symfony/css-selector | v7.3.0 | v7.4.0-BETA1 |
| symfony/dom-crawler | v7.3.3 | v7.4.0-BETA1 |
| symfony/lock | v7.3.4 | v7.4.0-BETA1 |
+----------------------+--------+--------------+
I think also we should add attribute support, as autowiring works elsewhere with attributes users will expect it to be the same here.
Added a question about private Vs protected, I think we perhaps have a valid case for a private property here.
> Every even-numbered .4 release is designated as an LTS.
Where does it say this? On the contrary it appears they call every .4 release an LTS?
And this still makes little difference to Drupal:
Drupal 10 will be EOL in December 2026. Symfony 6.4 will be EOL in November 2027.
We haven't announced an exact date yet but Drupal 11 is likely to be EOL in December 2028. Symfony 7.4 will be EOL in November 2029.
Drupal 12 dates haven't been confirmed, but neither has an end date been set for Symfony 8.
Even if we extended for another eleven months to meet Symfony, we also have to consider our other dependencies such as CKEditor, who won't necessarily make the same promises.
This is all information that hasn't changed since the last issue where you asked for the EOL to be extended, so I'm not sure of the point of rehashing it all again here; you will get the same answer.
Before we could do this we would need confirmation from Symfony of which versions will be LTS, also many years in advance; can you link to any Symfony policy statements where this has been announced? If not, I don't see how we can promise anything up front.