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

add support for Rector annotation-to-attribute-migration #72

Closed
Richard87 opened this issue Nov 17, 2021 · 8 comments
Closed

add support for Rector annotation-to-attribute-migration #72

Richard87 opened this issue Nov 17, 2021 · 8 comments

Comments

@Richard87
Copy link
Contributor

Hi

the parameter $route dissapeared in 6f4a45372d1c684cd5b4cea545712e6563f04e7d line 70.

This breaks automatic transform @annotation to #[Attribute] with Rector for users that use that parameter

@rvanlaak
Copy link
Collaborator

IMHO the $routeName, $routeParameters and $routeAbsolute parameters should be preferred over having a duplicate $route array with identical functionality.

Can you share and update the Rector script accordingly?

@Richard87
Copy link
Contributor Author

Richard87 commented Nov 17, 2021

No, the rector script is standardised, and just converts what is already used in the annotation.

Since the annotation allows "route": {"name": "xyz", "parameters": "abc"}, the atrtribute must allow it to, or the conversion will fail.

I recommend we deprecate the route param, and remove it in a future v. 2 instead

EDIT: Using the route param is also the only documented example: https://github.com/APY/APYBreadcrumbTrailBundle/blob/master/src/Resources/doc/annotation_configuration.md#route

@Richard87
Copy link
Contributor Author

This is the recor script:

return static function (ContainerConfigurator $containerConfigurator): void {
    // get parameters
    $parameters = $containerConfigurator->parameters();
    $parameters->set(Option::PATHS, [__DIR__ . '/src']);
    $parameters->set(Option::SYMFONY_CONTAINER_XML_PATH_PARAMETER, __DIR__ . '/var/cache/dev/App_KernelDevDebugContainer.xml');
    $parameters->set(Option::AUTO_IMPORT_NAMES, true);
    $parameters->set(Option::IMPORT_SHORT_CLASSES, false);

    $services = $containerConfigurator->services();
    $services->set(AnnotationToAttributeRector::class)->call('configure', [
        [
            AnnotationToAttributeRector::ANNOTATION_TO_ATTRIBUTE => ValueObjectInliner::inline([
                new AnnotationToAttribute(\APY\BreadcrumbTrailBundle\Annotation\Breadcrumb::class),
            ]),
        ],
    ]);
};

Basically just tels the rector script AnnotationToAttributeRector to convert Breadcrumb annotation

@rvanlaak
Copy link
Collaborator

Is there a way to hook into the configuration and alter the data before the attribute gets constructed?

For clarity, as your comment refers to a Rector upgrade script which is not part of this bundle (yet), this issue is not an issue with this bundle's code.

@rvanlaak rvanlaak changed the title 1.7.0.beta.1 route parameter disappeared, breaking rector auto-migration add support for Rector annotation-to-attribute-migration Nov 18, 2021
@Richard87
Copy link
Contributor Author

Not that i have found... I think doctrine have made their own script (pr into rector), but without that, and if we want to use rector, we need the route parameter...

@Richard87
Copy link
Contributor Author

Also, the rector script is not ment to be included in the bundle, its just a one-og script designed to tell rector how to transforme the code, and then to be deleted again (it might be relevant for documentation tho to explain how to simply use attributes).

@rvanlaak
Copy link
Collaborator

Not including it with the bundle is ok with me, feel free to close this issue in that case. Thanks

@Richard87
Copy link
Contributor Author

Yes, the problem is that the tool will fail if the attributes desnt have the argument $route.

And its a very handy tool 😁

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 a pull request may close this issue.

2 participants