- Issue created by @kim.pepper
- Merge request !6963#3426506 Create enums for File exists behaviours and deprecate consts β (Closed) created by kim.pepper
- Status changed to Needs review
6 months ago 3:21am 8 March 2024 - Status changed to RTBC
6 months ago 3:56pm 8 March 2024 - πΊπΈUnited States smustgrave
Searched for
FileSystemInterface::EXISTS_REPLACE = all instances replaced
FileSystemInterface::EXISTS_RENAME = all instances replaced
FileSystemInterface::EXISTS_ERROR = all instances replacedCR is there and makes sense.
LGTM
- π¦πΊAustralia mstrelan
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.
- π¦πΊAustralia kim.pepper πββοΈπ¦πΊSydney, Australia
Switched to PascalCase as that seems to be the consensus in #3339746: Decide on a coding style for PHP Enumerations β
- Status changed to Needs work
6 months ago 2:59am 11 March 2024 - Status changed to Needs review
6 months ago 7:27am 12 March 2024 - π¦πΊAustralia kim.pepper πββοΈπ¦πΊSydney, Australia
Chatted with @mstrelan in slack and per π Create enum for FilterInterface constants Needs work we should try and avoid using an int-backed enum.
- π¦πΊAustralia kim.pepper πββοΈπ¦πΊSydney, Australia
We were expecting only int constant values already, so I don't think that would affect anyone?
- π¦πΊAustralia mstrelan
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.
- π¦πΊAustralia kim.pepper πββοΈπ¦πΊSydney, Australia
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.
- π¦πΊAustralia kim.pepper πββοΈπ¦πΊSydney, Australia
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.
- πΊπΈUnited States smustgrave
So wonder if the current MR should be pointed at 10.3 and a new MR for 11.x with the deprecated code removed?
- Status changed to RTBC
6 months ago 3:46pm 19 March 2024 - πΊπΈUnited States smustgrave
Actually guess that could impact the history so probably a bad idea.
- Status changed to Needs work
6 months ago 5:53pm 19 March 2024 - π¬π§United Kingdom alexpott πͺπΊπ
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.
- Status changed to Needs review
6 months ago 4:25am 21 March 2024 - π¦πΊAustralia kim.pepper πββοΈπ¦πΊSydney, Australia
Changed deprecation to drupal 12.
- Status changed to RTBC
6 months ago 1:48pm 21 March 2024 - πΊπΈUnited States smustgrave
That's my bad, still trying to find a good rule of thumb for 12 vs 11.
Change to 12 looks good though.
- Status changed to Needs work
5 months ago 11:24am 30 March 2024 - π¬π§United Kingdom alexpott πͺπΊπ
I've added a comment the MR. I think we can be a bit looser in our expectations and preserve BC a bit more.
- Status changed to Needs review
5 months ago 11:21pm 31 March 2024 - π¦πΊAustralia kim.pepper πββοΈπ¦πΊSydney, Australia
Addressed feedback.
- Status changed to RTBC
5 months ago 11:13pm 5 April 2024 - πΊπΈUnited States smustgrave
Appears additional feedback has been addressed and all threads resolved.
- Status changed to Needs work
5 months ago 3:59pm 6 April 2024 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.
- Status changed to RTBC
5 months ago 12:02am 9 April 2024 - π¦πΊAustralia kim.pepper πββοΈπ¦πΊSydney, Australia
Rebased and test green so back to RTBC
- Status changed to Fixed
5 months ago 7:48am 9 April 2024 - π¬π§United Kingdom alexpott πͺπΊπ
Committed and pushed 830703f83bd to 11.x and e632e0980a6 to 10.3.x. Thanks!
-
alexpott β
committed 24807f9c on 10.3.x
Issue #3426506 by kim.pepper, smustgrave, mstrelan, alexpott: Create...
-
alexpott β
committed 24807f9c on 10.3.x
-
alexpott β
committed 8ae0a023 on 11.x
Issue #3426506 by kim.pepper, smustgrave, mstrelan, alexpott: Create...
-
alexpott β
committed 8ae0a023 on 11.x
-
alexpott β
committed 4b246b2d on 10.3.x
Issue #3426506 follow-up by alexpott: Create enums for File exists...
-
alexpott β
committed 4b246b2d on 10.3.x
- π¬π§United Kingdom alexpott πͺπΊπ
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 - πΊπΈUnited States mglaman WI, USA
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)
Automatically closed - issue fixed for 2 weeks with no activity.