Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add rector rule for format_size in 10.2 #286

Merged
merged 3 commits into from
Jan 11, 2024

Conversation

timohuisman
Copy link
Contributor

Description

The format_size() function in common.inc is deprecated in favour of using \Drupal\Core\StringTranslation\ByteSizeMarkup::create() that returns TranslatableMarkup

To Test

Drupal.org issue

https://www.drupal.org/node/3413972

@timohuisman
Copy link
Contributor Author

timohuisman commented Jan 11, 2024

I guess the tests are failing because of #285


return static function (RectorConfig $rectorConfig): void {
$rectorConfig->sets([
SymfonyLevelSetList::UP_TO_SYMFONY_63,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is the same as 10.1, so we dont need this in here.

@bbrala
Copy link
Collaborator

bbrala commented Jan 11, 2024

Small little feedback, otherwise, great work!

@timohuisman
Copy link
Contributor Author

Thanks for the review and the guidance @bbrala, I've resolved your feedback.

Copy link
Collaborator

@bbrala bbrala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great! Thank you :)

@bbrala bbrala merged commit aa11559 into palantirnet:main Jan 11, 2024
7 checks passed
@timohuisman timohuisman deleted the feature/3413972-format_size branch January 11, 2024 21:13
@AlexSkrypnyk
Copy link

AlexSkrypnyk commented Jan 11, 2024

@bbrala
This change does not have a fence for already replaced code. As a result - already processed code is reported as "needs fixing". Applying the fix again, results in another replacement, leading to unlimited requests to a replacement.

--- a/web/themes/contrib/civictheme/includes/utilities.inc
+++ b/web/themes/contrib/civictheme/includes/utilities.inc

-    'size' => DeprecationHelper::backwardsCompatibleCall(\Drupal::VERSION, '10.2.0', static fn() => format_size($file->getSize()), static fn(): TranslatableMarkup => ByteSizeMarkup::create($file->getSize())),
+    'size' => DeprecationHelper::backwardsCompatibleCall(\Drupal::VERSION, '10.2.0', static fn(): mixed => DeprecationHelper::backwardsCompatibleCall(\Drupal::VERSION, '10.2.0', static fn() => DeprecationHelper::backwardsCompatibleCall(\Drupal::VERSION, '10.2.0', static fn() => format_size($file->getSize()):

I think the CI needs to be updated to run the check twice to make sure that re-applying the rules on already fixed code does not trigger another need for a fix.

UPDATE: created issue in D.O. https://www.drupal.org/project/rector/issues/3414169

@bbrala
Copy link
Collaborator

bbrala commented Jan 12, 2024

Yeah, this is not this change though, but the overarching change which supports the bc calls. Added comment tot he issue. My collegue promised to have a look at whats broken, so might be able to fix it pretty soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants