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

Fix empty name in @Route #3246

Closed
wants to merge 1 commit into from
Closed

Fix empty name in @Route #3246

wants to merge 1 commit into from

Conversation

TomasVotruba
Copy link
Member

@TomasVotruba TomasVotruba commented Apr 24, 2020

@@ -118,9 +118,8 @@ public function __toString(): string
'path' => $this->printValueWithOptionalQuotes('path', $this->path, $this->localizedPaths),
];

if ($this->name) {
Copy link
Contributor

@stephanvierkant stephanvierkant Apr 24, 2020

Choose a reason for hiding this comment

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

Maybe if ($this->name !== null) {? A name attribute isn't required, but can be empty.

It fixes this bug, but I'm not sure about side effects.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if annotation parser from Doctrine makes any difference about these 2 cases.

Why do you need empty attribute?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's the point of this issue, isn't it? Name may be empty, but in this case an empty name is removed.

Copy link
Member Author

@TomasVotruba TomasVotruba Apr 24, 2020

Choose a reason for hiding this comment

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

I mean why do you need empty attribute in your code?

$this->redirect('')

It might not be technically possible to differentiate, so if that has no effect, it should be removed in your code.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, a method @Route can have an empty name ("", not null) if the class @Route has a name.

Copy link
Member Author

@TomasVotruba TomasVotruba Apr 24, 2020

Choose a reason for hiding this comment

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

How can you use that compared to removing the attribute?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand you correctly.

When you remove the name attribute from a method, the method is removed from routing. It should be empty, not removed. See my example: https://github.com/rectorphp/rector/pull/3240/files

That file contains valid code and should'nt be changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

See #3248

@TomasVotruba
Copy link
Member Author

Replaced by #3248

@TomasVotruba TomasVotruba deleted the fix-name-empty branch April 24, 2020 23:31
TomasVotruba added a commit that referenced this pull request Dec 29, 2022
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.

Rector removes empty name from @Route annotation
2 participants