Problem/Motivation

Cores FileSystemInterface::basename() can cause container circular references with contrib.
#3259065: Circular reference detected for service "s3fsfileservice" with mime_type guesser
#3529457: Copy core 11.1.x ExtensionMimeTypeGuesser to s3fs

FileSystem::basename()is essentially a duplication of PHP native basename()

PHP Native basename() is path unware, it functions on a path based on the directory separator.

It appears this was intended to work around https://bugs.php.net/bug.php?id=37738 however that issue no longer appears to be present in PHP as of the 8.x branch https://3v4l.org/XcflK

It makes little to no sense for basename() logic to be overridden by contrib. Any such override inside Drupal will not extend to 3rd party libraries. Even Symfony makes 27 calls to the native basename() function (I can not assert any of these methods are used by Core).

Steps to reproduce

Proposed resolution

Remove FileSystemInterface::basename()
Optionaly: Retain FileSystem::basename() in a trait that can be included when necessary (although I see no need to do this).

Remaining tasks

User interface changes

None

Introduced terminology

None

API changes

FileSystemInterface::basename() deprecation/removal

Data model changes

None

Release notes snippet

FileSystemInterface::basename() is now deprecated, use basename() instead.

CommentFileSizeAuthor
#19 3530461-nr-bot_hdofq9pv.txt91 bytesneeds-review-queue-bot
#15 3530461-nr-bot_ljq9yva7.txt91 bytesneeds-review-queue-bot
#8 3530461-nr-bot.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3530461

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

cmlara created an issue. See original summary.

cmlara’s picture

cmlara
he/him
commented
Version: 11.2.x-dev » 11.x-dev
cmlara’s picture

cmlara
he/him
commented
Issue tags: +Needs subsystem maintainer review

Would be good to have a subsystem maintainer sign-off before any effort is put into code on this.

Pinging @kimb0 (Kim Pepper) on Slack.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented

Looks like you are correct. I'd be happy to deprecate/remove.

Confirmed that:

  • the bug linked in the comment is no longer relevant.
  • Directory separators are handled natively
  • The behavior of php basename() seems to handle directories ending in slash just fine.
  • Stream wrappers work fine
   * PHP's basename() does not properly support streams or filenames beginning
   * with a non-US-ASCII character.

@see http://bugs.php.net/bug.php?id=37738

Here's an 3v4l snippet to test the standard basename() behavior:

https://3v4l.org/sqDsj

cmlara opened merge request !12403

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Active » Needs review

I reviewed the changes and confirmed that all use of FileSystemInterface::basename() have been converted to basename().

My only question is on which version this will be removed in.

@deprecated in drupal:11.3.0 and is removed from drupal:13.0.0.

Is it too late to have it removed in drupal 12.0.0 ?

cmlara’s picture

cmlara
he/him
commented

@kim.pepper Thanks for the quick review. Was typing up the following while your review came in:

This was actually not as embedded in core as I expected it to be.

Set deprecation for 11.3 removal in 13.0 based on (the not yet accepted) discussion in #3518671: [policy, no patch] Defer disruptive 11.3 deprecations for removal until 13.0 out of concern it might be considered 'disruptive' (removing a method).

As an argument it is not disruptive: basename()is PHP native and works in PHP8.1 which is the minimum supported version for any D10.x release. A module could easily support ^10 || ^11 || ^12 with just the native function.

Above my position to make the final call on D12 vs D13 (willing to edit to D12 if it is approved).

This does impact a deprecation added in #2311679: Separate MIME type mapping from ExtensionMimeTypeGuesser the file_system was added to the ExtensionMimeTypeGuesser as optional in D11.2 and would have been required in D12 will now be removed in D11.3.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3530461-nr-bot.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

cmlara’s picture

cmlara
he/him
commented
Status: Needs work » Needs review

Back to NR after the branch was rebased.

Never received a merge conflict email, did this actually have a conflict or was this a bot FP?

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented

It was a bot triggered comment that sent an email. I just rebased it.

nicxvan’s picture

nicxvan commented
Status: Needs review » Needs work

I think this can be deprecated for removal in 12.

Disruptive deprecations are defined here: https://www.drupal.org/about/core/policies/core-change-policies/allowed-...

I think a deprecation that is literally a find and replace and the replacement supports 11+ is not disruptive.

Further since this removes a deprecation for the negotiator I think you need to update and insert the message there and say it's not allowed, some people may have updated.

We can't just remove that argument without deprecation since it was released.

I would also update the original cr with a note linking to this issue to follow since it's something that will only exist for a minor point

cmlara’s picture

cmlara
he/him
commented
Status: Needs work » Needs review

No conflict rebased on orgin/11.x
Deprecation set to D12.0
Deprecation added for constructor argument
Original related CR referenced to this issue

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented

Reviewed this again. I think it is good to go, but will leave as NR for others.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs subsystem maintainer review +Needs Review Queue Initiative

Removing the sub-maintainer tag since this was worked on by the actual sub-maintainer :)

Did a search for "->basename(" and only instance is coming from the deprecation test
Issue summary is clear why this change can happen.
Reviewed both CRs and short and sweet. Kinda funny we made it required in 11.2 and not needed in 11.3 lol

Seems like a good replacements and no issues that I can see.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3530461-nr-bot_ljq9yva7.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs work » Reviewed & tested by the community

Rebased on 11.x

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Reviewed & tested by the community » Needs work
kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs work » Reviewed & tested by the community

Random fail

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3530461-nr-bot_hdofq9pv.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs work » Reviewed & tested by the community

Rebase on 11.x

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Reviewed & tested by the community » Needs review
mstrelan’s picture

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

Thanks for that. I looked through all the classes calling FileSystem::basename to ensure there were no leftover filesystem properties that were no longer in use. Can confirm there are none remaining.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented

I added DeprecatedServicePropertyTrait for the removed $fileSystem property.

catch’s picture

catch
he/him
Primary language English
commented
Title: Remove FileSystemInterface::basename() and use PHP native basename() » Deprecate FileSystemInterface::basename() and use PHP native basename()
catch’s picture

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

The constructor deprecations can be for removal in 12.x, we don't need to keep that around a full major release. I think the deprecation of the basename() method itself makes sense for removal in 13.0 though.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs work » Reviewed & tested by the community

Updated deprecations message in two constructors (DirectoryWithMetadataDiscovery and DirectoryWithMetadataPluginDiscovery to be for removal in 12.x.

I think I'm ok to put back to RTBC in this case.

catch closed merge request !12403

catch’s picture

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

Thanks. I overlooked in my first pass that we're removing a deprecation we added in 11.2 - good reason not to keep them around for two majors given how often we're adding/removing parameters from constructors.

Committed/pushed to 11.x, thanks!

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.

  • catch committed b919e480 on 11.x 
    Issue #3530461 by cmlara, kim.pepper, nicxvan, smustgrave, mstrelan:...

Status: Fixed » Closed (fixed)

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