Create enums for File exists options and deprecate consts

Searched for

FileSystemInterface::EXISTS_REPLACE = all instances replaced
FileSystemInterface::EXISTS_RENAME = all instances replaced
FileSystemInterface::EXISTS_ERROR = all instances replaced

CR is there and makes sense.

LGTM

Wondering if we need to postpone on #3339746: Decide on a coding style for PHP Enumerations → . My current preference is for UpperCamelCase but mostly just whatever is consistent throughout core.

Switched to PascalCase as that seems to be the consensus in #3339746: Decide on a coding style for PHP Enumerations →

Some feedback in the MR.

Chatted with @mstrelan in slack and per 📌 Create enum for FilterInterface constants Needs work we should try and avoid using an int-backed enum.

We were expecting only int constant values already, so I don't think that would affect anyone?

We were expecting only int constant values already, so I don't think that would affect anyone?

Well then we should add the param type to all the other functions as well.

But it's possible that someone was passing '1' instead of 1 (or the const) and that will now break if the calling class has strict types enabled. Maybe that's enough of an edge case that we can get away with it.

Well then we should add the param type to all the other functions as well.

Yeah, I meant to. Looks like I missed one or two.

Discussed with @mstrelan in slack and we can't add type hints. We need to add them as commented out typehints for DebugClassLoader to throw a future deprecation warning.

So wonder if the current MR should be pointed at 10.3 and a new MR for 11.x with the deprecated code removed?

Actually guess that could impact the history so probably a bad idea.

I think we should defer the removal of the deprecated code to Drupal 12 as per https://www.drupal.org/project/drupal/issues/3410492 🌱 Defer disruptive 10.3 deprecations for removal until 12.0 Active - this has a lot of usage and history. Giving modules a whole release cycle to update feels like a good idea.

Changed deprecation to drupal 12.

That's my bad, still trying to find a good rule of thumb for 12 vs 11.

Change to 12 looks good though.

I've added a comment the MR. I think we can be a bit looser in our expectations and preserve BC a bit more.

Addressed feedback.

Appears additional feedback has been addressed and all threads resolved.

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.

Rebased and test green so back to RTBC

Committed and pushed 830703f83bd to 11.x and e632e0980a6 to 10.3.x. Thanks!

This broke PHPStan on 10.3 because PHPStan triggered a usage of deprecated constant in the following code:

 * @deprecated in drupal:10.2.0 and is removed from drupal:11.0.0. There is no
 *   replacement.
 *
 * @see https://www.drupal.org/node/3223362
 */
function system_retrieve_file($url, $destination = NULL, $managed = FALSE, $replace = FileSystemInterface::EXISTS_RENAME) {

Imo that shouldn't trigger an error because system_retrieve_file() is already deprecated but it did... will ping @mglaman as this seems similar to https://github.com/phpstan/phpstan-deprecation-rules/issues/107

Smells like a bug in phpstan-deprecation-rules, too bad the phpstan playground doesn’t include the deprecated rules. but it’s easy enough to produce and submit an issue.

https://github.com/phpstan/phpstan-deprecation-rules/blob/1.2.x/src/Rule...

		$function = $scope->getFunction();
		if ($function !== null && $function->isDeprecated()->yes()) {

It seems $function is null because it’s not inside of the function (yet)

kim.pepper → closed merge request !6963

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