Problem/Motivation

There's todo in codebase pointing there
It was added because core required PHP < 5.4 before and can't use RecursiveCallbackFilterIterator

https://git.drupalcode.org/project/drupal/-/blob/9.1.x/core/lib/Drupal/C...

 * @todo Use RecursiveCallbackFilterIterator instead of the $acceptTests
 *   parameter forwarding once PHP 5.4 is available.
 *   https://www.drupal.org/node/2532228

Proposed resolution

Use iterator to filter tests instead of separate method (needs to detail)

Remaining tasks

patch, review, commit

User interface changes

no

API changes

TBD

Data model changes

no

Release notes snippet

no

CommentFileSizeAuthor
#32 2532228-32.patch9.6 KBandypost
#32 interdiff.txt2.53 KBandypost
#29 2532228-29.patch9.6 KBandypost
#29 interdiff.txt814 bytesandypost
#28 2532228-28.patch9.39 KBandypost
#28 interdiff.txt789 bytesandypost
#26 interdiff.txt3.48 KBandypost
#26 2532228-26.patch9.46 KBandypost
#24 2532228-24.patch7.75 KBquietone
#24 diff-22-24.txt790 bytesquietone
#23 2532228-22.patch7.78 KBandypost
#23 interdiff.txt7.08 KBandypost
#11 2532228-11.patch7.35 KBberdir
#3 2532228-3.patch7.34 KBdawehner

Issue fork drupal-2532228

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

markdorison’s picture

markdorison
he/him
commented
Version: 8.1.x-dev » 8.3.x-dev
Status: Needs work » Active
dawehner’s picture

dawehner
Primary language German
commented
Status: Active » Needs review
StatusFileSize
new 2532228-3.patch7.34 KB

Let's see whether this works.

I left \Drupal\Core\Extension\Discovery\RecursiveExtensionFilterIterator in for now, maybe someone used to use it in some other context. For the new class I tagged it with @internal, because its really just a detail.

Status: Needs review » Needs work

The last submitted patch, 3: 2532228-3.patch, failed testing.

markhalliwell’s picture

markhalliwell
Primary language English
commented
Related issues: +#2839191: RecursiveExtensionFilterIterator skips valid extensions because of blacklist

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new 2532228-11.patch7.35 KB

Still applied with git apply -3, just a reroll.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Related issues: +#2783389: There should be a way to add to the RecursiveExtensionFilterIterator whitelist, +#2209307: Convert database drivers into a regular extension type

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

neclimdul’s picture

neclimdul
He/Him
commented
Related issues: +#3151092: Replace use of whitelist/blacklist in \Drupal\Core\Extension classes

Relating blacklist property changes. Will need a reroll and updates after it lands.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue summary: View changes

updated IS, re-queued last patch

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue summary: View changes

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Parent issue: » #3179048: [meta] Remove dead code/unneeded compatibility layers now that PHP 8.1 is required

Parenting this under the PHP 8.1 meta, we definitely no longer support PHP 5.3 :)

andypost’s picture

andypost
he/him
Primary language Russian
commented
Version: 9.5.x-dev » 10.0.x-dev
StatusFileSize
new interdiff.txt7.08 KB
new 2532228-22.patch7.78 KB

rework patch a bit

quietone’s picture

quietone commented
StatusFileSize
new diff-22-24.txt790 bytes
new 2532228-24.patch7.75 KB

This is just a reroll.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

Seems to be having some build issues. Not seen that one before.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Version: 10.0.x-dev » 10.1.x-dev
Status: Needs work » Needs review
StatusFileSize
new 2532228-26.patch9.46 KB
new interdiff.txt3.48 KB

Update for 10.1 and added deprecation test, locally it works))

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Needs work 
--- Expected
+++ Actual
@@ @@
-'core/modules/mysql/src/Driver/Database/mysql/'
+'subdirectory/subdirectory/subdirectory/subdirectory/subdirectory/subdirectory/subdirectory/subdirectory/subdirectory/subdirectory/subdirectory/subdirectory/subdirectory/subdirectory/subdirectory/subdirectory/subdirectory/subdirectory/subdirectory/subdirectory/subdirectory/subdirectory/subdirectory/subdirectory/subdirectory/subdirectory/subdirectory/subdirectory/subdirectory/subdirectory/subdirectory/subdirectory/subdirectory/subdirectory/subdirectory/subdirectory/subdirectory/subdirectory/subdirectory/subdirectory/core/modules/mysql/src/Driver/Database/mysql/'

something is wrong, 7 tests failed

https://dispatcher.drupalci.org/job/drupal_patches/167702/testReport/

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt789 bytes
new 2532228-28.patch9.39 KB

This way it pass locally Drupal\Tests\Core\Database\DatabaseTest and more inline with previous implementation

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new interdiff.txt814 bytes
new 2532228-29.patch9.6 KB

And a bit of clean-up

The last submitted patch, 28: 2532228-28.patch, failed testing. View results

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs change record

Deprecation test looks good.

Think this would need it's own change record though. https://www.drupal.org/node/3162143 doesn't appear to address what's happening here.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
Issue tags: -Needs change record
StatusFileSize
new interdiff.txt2.53 KB
new 2532228-32.patch9.6 KB

Added CR https://www.drupal.org/node/3343023

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Lightning speed!

Changes look good to me.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 32: 2532228-32.patch, failed testing. View results

smustgrave’s picture

smustgrave commented
Status: Needs work » Reviewed & tested by the community

Random ckeditor triggering tests again.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 32: 2532228-32.patch, failed testing. View results

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Reviewed & tested by the community

Requeued

quietone’s picture

quietone commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record updates

Doing RTBC queue triage.

The change record should have more detail. A before and after section with a code example is usually included for deprecations to help the developer. Adding tag.

+++ b/core/lib/Drupal/Core/Extension/Discovery/RecursiveExtensionFilterCallback.php
@@ -83,17 +81,21 @@ class RecursiveExtensionFilterIterator extends \RecursiveFilterIterator {
+  public function __construct(array $skipped_folders = [], $accept_tests = FALSE) {

Shall we type hint $accept_tests.

The deprecation style done here is for Abstract classes, interfaces, traits, and classes with a private constructor but the constructor is not private. I think this should be using the style for concrete classes.

I did not do a thorough review of the code.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

spokje’s picture

spokje
Primary language Dutch
commented
Assigned: Unassigned » spokje

Spokje opened merge request !4312

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Needs review
Issue tags: -Needs change record updates

Updated CR, switched to MR-workflow (note: first commit was a straight-up reroll of patch #32), type-hinted the living daylights out of all and everything and switched to the recommended deprecation-through-constructor.

I think I've addressed all concerns raised by @quietone in #38, so put this on NR.

spokje’s picture

spokje
Primary language Dutch
commented
Assigned: spokje » Unassigned
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Deprecation looks good and CR has the requested before/after section so moving back to RTBC.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs work

Some nitpicky comments, but I think we can perhaps remove the acceptTests() method entirely and simplify this a bit.

spokje’s picture

spokje
Primary language Dutch
commented
Assigned: Unassigned » spokje
spokje’s picture

spokje
Primary language Dutch
commented
Assigned: spokje » Unassigned
Status: Needs work » Needs review

Thanks @longwave, resolved all threads.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Seems the threads by @longwave have been resolved.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Fixed

Committed c343195 and pushed to 11.x. Thanks!

Also published the change record.

  • longwave committed c3431950 on 11.x 
    Issue #2532228 by Spokje, andypost, quietone, dawehner, Berdir,...

Spokje closed merge request !4312

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.