Skip to content

[SYMFONY 4.4] replace Link argument values with constants #812

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

Merged
merged 1 commit into from
Jul 15, 2025

Conversation

JohJohan
Copy link
Contributor

@JohJohan JohJohan commented Jul 10, 2025

use Rector\Config\RectorConfig;

// https://github.com/symfony/symfony/blob/4.4/UPGRADE-4.4.md

return static function (RectorConfig $rectorConfig): void {
$rectorConfig->ruleWithConfiguration(ReplaceArgumentDefaultValueRector::class, [
Copy link
Member

Choose a reason for hiding this comment

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

This should import file

symfony44-weblink.php then include here and in Set class

https://github.com/rectorphp/rector-symfony/blob/main/src/Set/SetProvider/Symfony4SetProvider.php

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👍

@@ -12,4 +12,5 @@
$rectorConfig->import(__DIR__ . '/symfony44/symfony44-http-kernel.php');
$rectorConfig->import(__DIR__ . '/symfony44/symfony44-templating.php');
$rectorConfig->import(__DIR__ . '/symfony44/symfony44-security-core.php');
$rectorConfig->import(__DIR__ . '/symfony44/symfony44-weblink.php');
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@samsonasik i see we have:

new ComposerTriggeredSet(
                SetGroup::SYMFONY,
                'symfony/symfony',
                '4.4',
                __DIR__ . '/../../../config/sets/symfony/symfony4/symfony44.php'
            ),

In the list and symfony44.php' has all the sub includes like mine symfony44-web-link.php and also symfony44-templating.php is this expected? Because you might not want to provide symfony44-web-link.php when you dont have the symfony/web-link package right? Should it be in symfony44.php'?

Copy link
Member

Choose a reason for hiding this comment

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

that's for upgrade by composer per component, so something like, see other list

new ComposerTriggeredSet(
                SetGroup::SYMFONY,
                'symfony/weblink',
                '4.4',
                __DIR__ . '/../../../config/sets/symfony/symfony4/symfony44-weblink,php'
            ),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check, i added that i was just wondering because it is also already in list config/sets/symfony/symfony4/symfony44.php so might be added when you dont have symfony/weblink or not?

Copy link
Member

Choose a reason for hiding this comment

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

that will auto detect when you have symfony/weblink in composer.json by component

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check, it should be ready for review i also updated the other pull requests

@samsonasik samsonasik merged commit c333930 into rectorphp:main Jul 15, 2025
5 checks passed
@samsonasik
Copy link
Member

Thank you @JohJohan

@JohJohan JohJohan deleted the SYMFONY44 branch July 15, 2025 15:24
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.

2 participants