Add support for D10.3

Notes on changes:

Added a NewS3fsFileSystem class. This will be used on D10.3+
If I had realized Drupal Core's API policy stated " the interface is treated as an API for callers but not for implementers. In more detail" and been better at defining API as it relates to the Drupal Ecosystem (I was mostly relying on a transient acceptance that Drupal Core's policy was s3fs as well) I would of marked the previous S3fsFileSystem class final to prevent extension. From quick testing the old class actually will still work with D10.3 however the protected method internally did strictly call for integers only and unlike core I don't agree with opening these up outside a major version. The replacement is a final class so that this isn't an issue in the future.

Added a NewS3fsImageStyleDownloadController (currently marked @internal and I'll probably make it final before this issue lands). This will be used on all deployments. The existing one extended an @internal class. We actually don't need the new feature added into core as we internally validate our routes and we are solely responsible for the s3fs provided routes and only s3fs routes.

Since the core class was @internal it could make breaking changes in a Minor release without consequence. We could easily adopt this change (or a variant of it) and have not issue inside of S3FS, however it would move the breaking change for anyone extending our class to include versions of Drupal core as far back as D8.8. I'm aware of sites that have extended the previous class in the past, they are going to break one way or another and need to make a code change.

This is extremely borderline from a semver standpoint. This is an attempt to choose the least damaging impact. The only argument in favor of this I can make is that "we extended an internal class and by extension we were internal as well" --- I really don't like that logic, its more of an excuse of why I'm not bumping us to a 4.x for this change and relates to our poorly documented definition of API.

Disclosure: I was the primary promoter of the Core change for the ImageStyleDownloadController that caused this break for security reasons.

Work I still need to do:
Cleanup new phpstan baselines related to these changes and validate any coding standard changes.

I think we are going to roll with this.

Tested back to D9.5/PHP 7.4 in https://git.drupalcode.org/issue/s3fs-3447227/-/pipelines/186921 (Gitlab CI for us is not setup to test further back at moment)

I don’t want to exclude the old image style generating class so we are stuck with a single PHPStan error.

Long term we probably need a config file that varies based on the version of Drupal Core however I think I will make that a followup issue.

I've had a look through the changes as a whole, and they seem ok except for two thoughts:

• The Core Version Requirement line could be optimised a bit using > and < version operators.
• I usually try to avoid calling things "New-X" because inevitably their newness degrades over time... would it be possible to rename the new class by drawing from the new differences?

I would really like to see this committed ASAP because it's part of a complex version dependency ring on my current site.

I would really like to see this committed ASAP because it's part of a complex version dependency ring on my current site.

I had noted in Slack (but not here) that I hoped to commit this today. It is not as manually tested as I would normally like to do for a release, however at least the tests are passing.

I usually try to avoid calling things "New-X" because inevitably their newness degrades over time...

Good Point. NewS3fsFileSystem Could probably be renamed to S3fsFileSystemD103 or similar since it is is for D10.3+. NewS3fsImageStyleDownloadController I’m less sure of a descriptive name as it completely replace the existing controller and will not be D10.3 specific.

The Core Version Requirement line could be optimised a bit using > and < version operators.

True, though I do not believe the current syntax is causing any issues is it? IIRC the core logic is simplistic compared to composer. I believe it is safe to defer this to a followup issue so we don’t accidentally breach the constraints limitations by rushing it.

Could not think of a better name for the ImageStyleDownloadController. Its internal and final so we can rename if we ever need to.

Did rename src/NewS3fsFileService.php to src/S3fsFileSystemD103.php.

Committing and tagging a release shortly.

I was pinged about this module for D11 compatibility -- while I certainly appreciate the efforts of BC, some of these changes make me scratch my head a bit.

Is the root issue here trying to maintain Drupal 8.8 compatibility in the current major? Because if so, thats not valid -- You can and should drop Drupal 8 support in this minor release. The minimum should be 9.5. You do not need to change the major version to drop earlier support as semver will not allow the module to be installed on older sites.

Also, looking at #3324737: Do not support core versions newer than current published beta or higher → I see the concern for implicit support for the 'next minor' -- however it is more the responsibility for core not to break modules in a minor release vs expecting modules to check each minor for stability. It appears that approach prevailed and there was no rework needed for s3fs. But I'd also suggest that using gitlab-ci you should know if the next version will break and make changes accordingly while its still in beta. We've seen some breaks with Drupal 10.3 coming out across the ecosystem, but having composer be lenient allows folks to fix the specific issue rather than being stopped by composer for no reason. And again, stability is more core's responsibility, so any change that breaks modules should be considered core bugs.

Added a NewS3fsImageStyleDownloadController (currently marked @internal and I'll probably make it final before this issue lands). This will be used on all deployments. The existing one extended an @internal class. We actually don't need the new feature added into core as we internally validate our routes and we are solely responsible for the s3fs provided routes and only s3fs routes.

Do you mean? ImageStyleDownloadController This is not marked @internal. I'm struggling to see why this should have been refactored to work with Drupal 10.3? If it was just the constructor, that can easily be made to work with Drupal 9.5 through 11 without going to the extreme this issue did.

Same with S3fsFileService -- I don't see anything in there that must be changed. I see some new coding standards you'd like to use, but again -- those aren't needed for D10.3 (or 11) compatibility.

The end result here is a mess of files and maintainability, making it that much harder to achieve clean Drupal 9.5, 10, and 11 compatibility.

Is the root issue here trying to maintain Drupal 8.8 compatibility in the current major? Because if so, thats not valid -- You can and should drop Drupal 8 support in this minor release. The minimum should be 9.5. You do not need to change the major version to drop earlier support

While I know a sizeable population of D.O. contrib maintainers hold that opinion I personally (along with other maintainers) do not.

My rule is that if I install 3.x on a version of Drupal I should be able to install ANY 3.x on that release of Drupal. Addtionaly in my eyes it is important to realize we are not actually developing for a specific release of Drupal Core, we are developing for an API spec defined by Drupal Core. No one should be running stock Drupal 8.8, howeve much like D6 continued on and D7 appears it will continue on after their EOL's its probable to assume that somewhere a D8 site does the same by back-patching core security fixes and changes. This is what it means to support an 'operating enviroment'.

Now is a good time for me to note, I am primarily a Security Engineer, followed by a Systems Admin, followed by a Web Dev. My view on these matters comes from my own personal history of dealing with software deployments and customer deployments of complex solutions as such I don't look at these decisions from a "How easy is it for me as a maintainer" I look at them as "How much pain is this causing my end user who is out of date trying to update to my latest release"

Do you mean? ImageStyleDownloadController This is not marked @internal. I'm struggling to see why this should have been refactored to work with Drupal 10.3?

I mention this in other issues. I've started to realize many(most) developers do not realize just how much of Drupal Core is NOT API. Drupal Core is essentially a "Internal by default" specification. Core controller routes like the ImageStyleDownloadController are "not part of the API of the module unless specifically marked with an @api tag on the method or class" per the Core BC policy. I'll note we have a side discussion in Slack #phsptan https://drupal.slack.com/archives/C033S2JUMLJ/p1718388676560139 where we actually shock ourselves in how some classes that may be frequently abused are techcnaly not recomended to do so (and anyone doing so accepts that a patch release may break their entire module)

As noted in the other issues, this was a security change in core, that alone is grounds to breach Backwards Compatibility per Drupal Core's policy. Addtionaly as noted as a Controller route core had no requirement to provide BC in the first place. They strive to do so, but they have not requirement to do so. I strongly stand behind the change in core. I personally observed it provide an open proxy exploit on systems.

We made a mistake extending it in the past, I can't undo that. 4.x was already going to not extend the core class to allow us to avoid the issues (especially since we called none of the code from the core provided class).

With the changes in 10.3 including a method signature change for deliver() we really didn't have much choice there was going to be some sort of break with 10.3, the question is how big a break.

Yes I could add the extra NULL on the method and gain compatibility with Drupal 10.2 and 10.3 (as long as no one extends us) and for all direct callers, however the impact is now that anyone extending us on the currently supported core 10.2 release will be broken. By creating a new class and using it for all releases going forward we make the break be solely impacting to 10.3+ installs which are already broken. This falls in line with me aiming to cause my fellow systems admins as little pain as possible, pain for software belongs to the developers. This also tracks with my view on Majors that if your running a set release of Drupal Core and you update to the latest release in the same major your site will not break. I absolutely agree I'm pushing the limits of that with the "old" class not being upgraded to 10.3

Usage/Modification of non-api code are also why I limit us to specific minors. My responsibility as a developer is to only assert comparability I feel I can guarantee, and based on the history of Core (within their API specs) we can't guarantee a full Core Major support so I will not add a wide constraint that I can't back up. This is as much about site owners protection as its is my own mental self care, I don't want to be dealing with Drupal Core compatibility issues under Downtime Response procedures. We also often argue that "Well semver means they will get the latest release that is already compatible with D10.3" 1) is not always the case we meet that deadline (though I try for it this release cylce I've been in a spot where I could not spend long coding/testing sessions.) 2) We assume site owners will upgrade the module before upgrading core or at the same time as core, we can not guarantee that occurs with a wide constraint.

Same with S3fsFileService -- I don't see anything in there that must be changed. I see some new coding standards you'd like to use, but again -- those aren't needed for D10.3 (or 11) compatibility.

The real big issue here is that Core doesn't actually define an API for Interfaces for Implementers.

Core is are allowed to change interfaces in a breaking way as long as callers are not impacted. I often mainatin that now that strict typedefs are being used more often we will actually see more of these issues.

Callers are indeed not impacted, they can pass either older Integers or newer ENUMs without issue (other than deprecation messages) however for us as an implementer this actually changes our method signatures. I note above that yes the old class actually works because of the lack of strict typedefinitions, however this is technicaly a method signature change. Our code is working only because it does nothing that explicitly expected integers.

I tend to allign myself closer to Symonfy BC policy https://symfony.com/doc/current/contributing/code/bc.html#changing-classes where I don't view it as acceptable to increase the allowed scopes on those extending us. Core does it, and thats their right to set their API and BC policy, I just don't agree with it and do not use the same 'relxed' impact.

The remainder of the changes is twofold 1) A requirment that 'new code' (even a duplicated class) needs to be brought up to latest standards (including PHPStan error resolution that was previously baselined) which should not be skipped. 2) The syntax changes since it is know the class will only be used on PHP8 installs and doign so made importing the class use less boiler plate. You are correct we didn't have to change the syntax to PHP, I absolutly could have kept the older boilerplate in the conversion however that would in my opinion created a class that is harder to mainatin long term, it cost me a few minutes to make future me curse past me less.

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

cmlara → closed merge request !52