#35: this is a totally alternative approach, that removes the need for the other issue.
Too long names are automatically shortened here to match the db limitations.
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...
I think we are close to be able to do that. IMO the hardest part is to consolidate test time results from the multiple spawns of run-test.sh and again from the multiple CI jobs. Good news is that a PoC to get a consolidated artifact with all the data seem to work in ✨ Report usage of deprecation ignores during test runs Active . Once that's in, we could build on that logic to do the same for test time results.
POC works, see the output at the end of run-tests.sh execution... I "just" need to find a way to consolidate results across different jobs to have a single overall stats board.
Filed ✨ Report usage of deprecation ignores during test runs Active to explore #7.
mondrake → created an issue.
Not in general terms. What I am saying is that the use of final should be pondered on a case by case basis. And because of that, that we should not rule it out in principle.
For example, for me Drupal\Core\Database\Identifier\Table in
📌
[WIP] Decouple identifier management from database Connection, introduce an IdentifierHandler class
Postponed
is OK to be final because it minimally extends a Base class, it should be used only to handle a database table, and its values are managed by a factory that feeds them to the object based on the database backend (varies). You want to do something like a Table? Extend a new class from the base one.
OTOH, use of final in https://github.com/paratestphp/paratest/issues/939 makes no sense to me because how it's been used prevents any overriding of anything... it preaches composition but it does not have the logic behind. So you have only a wall there. Same applies to a lot of recent developments in the PHPUnit project.
For sure this is a case where less is more; but less does not need to be zero...
My 0.02.
I am +1 on using the @final annotation more widely - it has no runtime impact while at the same time static analysis catches it and we have ways to manage it without hard failures.
I am -1 on forbidding final keyword as a general rule. While I agree it's frequently misused (see this https://github.com/paratestphp/paratest/issues/939 for example - it's making paratest practically a no-go for Drupal), still using it is a matter of deliberate choice that should be agreed upon before commiting to the codebase. And IMHO there are cases where a class is just a front runner for other code that can be varied in a composition model (for example, Drupal's Database class).
Seems all good to me, we are narrowing the ignores so FTW.
Oh my... entered comment in the issue tags field...
Couple of nits inline, NW just to have a look
Thanks for reviews!
I'd love to add @phpstan-type, but I am reluctant because of
📌
Forbid limited length primary and unique keys, allow only in indexes
Needs work
and
✨
Support PHPStan's annotations for local type aliases and array shapes
Active
.
This tells me it would be great to find a way to report ignored deprecations that are no longer necessary… like PHPStan does. Will be hard though as PHPUnit tests are run in parallel jobs in Drupal and results would need some sort of consolidation.
+1 for the commit, nothing seems odd. Should some oddity show up, we can address from there IMHO.
In fact, generally speaking, I think we're overdoing with the standard process when it comes to dependency bumping (not in this case, though, there were a few code changes that are beyond the dependency increase).
Other projects automate this process (e.g. using Dependabot)
... a coding standards or coder rule change ...
see #3516489-6: @phpstan-ignore-next-line annotation collides with PHPCS → with link to an upstream PR by @jonathan1055. That would be sufficient IMHO.
Rebased. Since when this was RTBC, added logic for determining group metadata information under PHPUnit 11.
Rebased, merge conflicts solved for composer files. No changes. Back to RTBC.
Adjusted post- 🌱 [meta] Support PHPUnit 11 in Drupal 10 Postponed .
Letting PHPUnit's own deprecations fail the job execution on PHPUnit 8.5 as the purpose of that job is precisely to shout out when language deprecation or changes occur in this stage of PHP development, so I thought it is a good spot where to let PHPUnit shout as well.
With the PHPUNIT_FAIL_ON_PHPUNIT_DEPRECATION job variable it's a piece of cake to silence them if we do not manage to solve them before PHP 8.5 is released.
Looks like bot and your local have an old baseline version? I added the return type and the core CI job tells me to remove the above from the baseline
Filed 📌 Remove the Farbtastic color selector plugin Active .
mondrake → created an issue.
From the release note:
Known issues
- The Color backport → module does not have a Drupal 11 compatible release yet. If you are using the Farbtastic plugin for color selection, the 2.x-dev branch was tested and seems to work fine. If no Drupal 11 compatible release comes earlier, the Farbtastic plugin will be removed in next minor of Image Effects.
Where do you see a recommendation to use the dev version?
You can:
- change the color selector plugin from Farbtastic to HTML via the config UI
- upgrade to 4.0.0
The reason why 4.0.0 is still kind of working with the Farbtastic plugin is that there were people using it, but given that Color backport is nowhere, support will be removed in 4.1.0. Hopefully everyone will have moved away (given the release notes...) by that time.
Found some issues while testing this in combination with 🌱 [meta] Support PHPUnit 11 in Drupal 10 Postponed . Given that one is RTBC, let's postpone on that.
Changes made
mondrake → created an issue.
Hi, thanks
No that does not work - there's the PHPDoc ending token */ in an isolated line and PHPStan does not take it into account - it says that there are no errors to ignore on that line.
mondrake → made their first commit to this issue’s fork.
Let’s have opinions on the best naming from #13 before making changes.
Meanwhile, better wait for 🐛 [CI] Components tests coverage metrics differ by PHP version Active to get in.
How annoying. Hitting again 🐛 @phpstan-ignore-next-line annotation collides with PHPCS Active
composer update mglaman/phpstan-drupal phpstan/extension-installer phpstan/phpstan phpstan/phpstan-deprecation-rules phpstan/phpstan-phpunit
Removing the bleedingEdge setting as PHPStan 2.1.13 introduced Report @internal symbols usage from outside their top namespace as a bleeding edge feature. We do not have currently a case for bleeding edge being active, so IMHO we can defer a decision on what to do with the @internal rules (also, maybe PHPStan will evolve there too).
2.1.14 released today.
The MR is now splitting the unit tests in a stage of its own - still the new stage's jobs are set to start without waiting for the 'Lint' stage to end.
📌 Bump PHPStan and family to latest versions Active is ready and an intermediate step.
📌 Bump PHPStan and family to latest versions Active is ready and an intermediate step to the latest.
Sorry, I found opportunities to remove duplication
Ah! No it’s already for 8.4, check the MR - 8.3 and 8.5 are getting the “no-coverage” job settings. Anyway this is about components unit tests which are getting rather independent as we speak.
Why so?
Agree we should close that meta issue, made changes there accordingly.
wrt to the CR, I am unsure. tbh I find the as-is a bit weird, many implementations of the interface methods rely on those params that are not in the interface signature - to the point that I was confused, started trying to remove them in the implementations just to realize afterwards I should do the other way around i.e. adding them to the interfaces. So for me we are just getting things right as they should be here, not changing conceptually the API. My 0.02€.
I agree with #11, made IS changes so this is no longer a meta and once completed, we can close it.
Future changes, yes, following the process:
https://www.drupal.org/node/3376455 →
https://www.drupal.org/about/core/policies/core-change-policies/how-to-d... →
No brainer
FTR, weirdly, the revert also fixed the build tests failure lately in 🌱 [meta] Support PHPUnit 11 in Drupal 10 Postponed . Ideally, it would preferable to postpone this one on that if there is interest to get PHPUnit in 11.2 so that we do not have to play catch there.
The revert of 📌 Switch the default test environment to PHP 8.4 Active also fixed the build test failure... weird.
MR implements option 1 from the issue summary. IMHO option 2 is overkill and 3 is complicated and risk regressions easily.
from https://docs.phpunit.de/en/10.5/attributes.html:
PHPUnit will first look for metadata in attributes before it looks for annotations in comments. When metadata is found in attributes, metadata in comments is ignored.
Postponed on 🐛 Fix test annotations for tests introduced by #3485896 Active
There's a small problem with mixing test annotations and attributes on the same level, here. Impacting 📌 Deprecate TestDiscovery test file scanning, use PHPUnit API instead Active . Opened 🐛 Fix test annotations for tests introduced by #3485896 Active for a fix.
mondrake → created an issue.
yes I am reverting last commit now, but the failure then gets to the PHPStan job
It seems failures are now due to drupal/core-recommended not allowing to bump a dependency:
Your requirements could not be resolved to an installable set of packages.
Problem 1
- Root composer.json requires drupal/core-recommended ^11 -> satisfiable by drupal/core-recommended[11.2].
- drupal/core-recommended 11.2 requires sebastian/diff ~5.1.1 -> found sebastian/diff[6.0.2] but it does not match the constraint.
Looks like 📌 Switch the default test environment to PHP 8.4 Active has broken build tests for this MR.
Minor issue in a follow-up: 🐛 [CI] Components tests coverage metrics differ by PHP version Active .
mondrake → created an issue.
I think the build test failures under PHP 8.4 are somehow connected with the composer script that updates PHPUnit to 11, and probably caused by a permission problem.
Anoyone familiar with build tests that can lend a hand?
Is it really worth supporting global const defined in .module files as we speak? Could we not assume OOP consts only?
Whops… ok.
This is now failing with
Drupal\Core\Config\UnsupportedDataTypeConfigException: Invalid data type in config config_enum_test.settings, found in file core/modules/config/tests/config_enum_test/config/install/config_enum_test.settings.yml: The constant "CONFIG_ENUM_TEST_VALUE" is not defined at line 2 (near "bar: !php/const CONFIG_ENUM_TEST_VALUE"). in Drupal\Core\Config\FileStorage->read() (line 118 of core/lib/Drupal/Core/Config/FileStorage.php).
I suppose because CONFIG_ENUM_TEST_VALUE is a global const defined in config_enum_test.module, but the module file is not preloaded.
There are test failures under PHP 8.4.
Added CR draft.
Updated IS
@andypost I think that tag would be a call for a release manager, tagging.
Also #59 needs an answer, I will write a CR afterwards.
Here's another GitLab upstream issue to watch for child/parent pipeline relationship https://gitlab.com/groups/gitlab-org/-/epics/4019, looks like it's actively pursued.
Blocker went in.
No sensible difference, I think the change is small enough to let me self-RTBC this back.
Huh, did not even look at that, was just a copypasta exercise. Let's see with 2.
Merged wit 11.x HEAD, resolved conflicts and addressed PHPCS and PHPStan errors. Would love to see this in.
mondrake → made their first commit to this issue’s fork.
This is blocking 📌 SophronMimeTypeMapFactory should just override MimeTypeMapFactory::doCreateMap() Active .
Rebased