10000 Rename security-csrf `TokenStorageInterface` for better DX · Issue #58991 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Rename security-csrf TokenStorageInterface for better DX #58991

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.

Sign up for GitHub

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

Jump to bottom
Open
chalasr opened this issue Nov 26, 2024 · 16 comments
Open

Rename security-csrf TokenStorageInterface for better DX #58991

chalasr opened this issue Nov 26, 2024 · 16 comments
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) Security Status: Needs Decision

Comments

@chalasr
Copy link
Member
chalasr commented Nov 26, 2024

Description

When typing TokenStorage in your IDE and being autocompleted, what you're looking for is the TokenStorageInterface from security-core, not the one from security-csrf. Yet the csrf one is proposed first which has always been annoying. I propose to prefix that interface by Csrf for the sake of better DX.

Example

Before:

Screenshot 2024-11-26 at 10 58 14

After, one gets the Core TokenStorageInterface proposed first.
@xabbuh xabbuh added Security DX DX = Developer eXperience (anything that improves the experience of using Symfony) labels Nov 26, 2024
@kevinpapst
Copy link

Shouldn't that be feature request with Jetbrains instead, so the IDE remembers and sorts most often used references on top?

@chalasr
Copy link
Member Author
chalasr commented Dec 19, 2024

@kevinpapst I don't know if/how this could be implemented on PHPStorm's side. Also this applies to any IDE and having them remembering about the best choice still imply to train them so that wouldn't fix the experience on first time approaching Symfony.

@symfony/mergers Any though before someone start working on this? The renaming impact should be pretty low to me.

@derrabus
Copy link
Member

In order to work on this, we have first to solve one of the hardest problems in modern computer science: naming things.

@GromNaN
Copy link
Member
GromNaN commented Dec 19, 2024

naming things

CsrfTokenStorageInterface would be expressive.

@kevinpapst
Copy link

What I hoped to say between the lines: that sounds like a unneccessary "BC break for DX reasons" 😁

Let me ask you a question:
Bildschirmfoto 2024-12-19 um 13 40 49

When I try to import Request I have to go through a list of 5+ choices. The one from the HttpFoundation Package is never the first one, even though the one I want. Are we going to rename the BrowserKit one as well to get rid of it? And then? I still have to skip 3 other ones.
Let's assume I would only have the ones from Symfony: I'd bet, that my Request example happens more often than the TokenStorange example. Can we fix that one as well?

See, actually I don't care at all if you rename it, I just think that you are looking for the wrong solution. You are a core member and this might be accepted (therefor). But the greater good would be to open up a Feature request with (y)our favorite IDE. There are many Reddit posts talking about the same problem. Being able to sort by usage would be so much better than adjusting the Framework around the IDE.

@chalasr
Copy link
Member Author
chalasr commented Dec 19, 2024

Your reasoning sounds good to me generally speaking. It's all about finding the right balance though, with cost and impact (positive and negative) in mind.

As you pointed out renaming BrowserKit's Request wouldn't help for the Request issue as there'd still be 2 other conflicting classes that are out of our control. The TokenStorage one is pretty easy to sort out on the contrary, one interface to rename, small negative impact as far as I can tell.

For the same reason we should strive not to use the same naming multiple times in newly added code , which I think we do.

@yceruto
Copy link
Member
yceruto commented Dec 19, 2024

Note that you can currently filter by prefixing with Csrf:

image

@chalasr
Copy link
Member Author
chalasr commented Dec 19, 2024

@yceruto Right, you do that once you already know about the confusion though.

@derrabus
Copy link
Member
derrabus commented Dec 19, 2024 •
edited
Loading

@kevinpapst made a good point. The Request issue annoys me far more often than the token storage one and renaming each and every Request class can't possibly be the solution. Also, that strategy somewhat defeats the purpose of namespaces. Yet, with the prosed change we would open up the slippery slope to renaming more classes like Request.

If we rename a class or interface, we generate quite some busy work downstream and I'm not 100% sold that it's worth it just for the sake of having a more collision-free name.

Shall we open an issue with JetBrains? The IDE should be able to figure out which classes would be the most likely to be picked by the developer, based on previous behavior or general usage of classes inside the current codebase. If the IDE is made smarter in that regard, everybody wins.

@GromNaN
Copy link
Member
GromNaN commented Dec 19, 2024

I have constantly the same issue with TestCase. So renaming everything is not the solution.

@Nyholm
Copy link
Member
Nyholm commented Dec 19, 2024

Generally I am against this. We have namespace and Jetbrains should "be better".

However, let's be pragmatic and help users instead of being "correct".

I agree to rename the Csrf token storage.

@wouterj wouterj mentioned this issue Dec 23, 2024
Closed
@wouterj
Copy link
Member
wouterj commented Dec 23, 2024

I think there are a few important differences between the TokenStorage case and the Request case:

  • We control both classes in the TokenStorage case, whereas Request conflicts with classes outside of our control
  • The namespaces are really close. Of the 34 characters shown by the IDE, only the last 3 differ (and this namespace part still has the same length).
  • This is a bit of a hypothesis, but the CSRF token storage is mostly an internal concept. High level usage of CSRF protection will not interact with this interface directly. I expect the rename to not have that much of a burden on the community, whereas it would be with any of the 2 Request classes living in Symfony.

@OskarStark
Copy link
Contributor

I agree with @Nyholm here in this case let's rename and the IDEs should be better in the future 🙋‍♂️

@dunglas
Copy link
Member
dunglas commented Dec 23, 2024

Maybe a silly question, but couldn't we add some code in the Symfony IDEA plugin to force the classes we want to show up first?

@chalasr
Copy link
Member Author
chalasr commented Dec 23, 2024 •
edited
Loading

I thought you were more using VSCode lately isn't it? 😄 More seriously I don't know enough about Jetbrains plugin development to say if it's possible or not but I presume it is for PHPStorm and probably others, but maybe not all.
I'm proposing to take the quick path on this particular one and tackle it in core because I can't think of other similar cases, at least not ones that don't imply to make too much assumptions, so this should not create a precedent for a dead end.
No big deal here and I'm not happy to rename things either, but as explained the impact should be quite low in this case.

@kbond
Copy link
Member
6FD8
kbond commented Dec 23, 2024

Maybe a silly question, but couldn't we add some code in the Symfony IDEA plugin to force the classes we want to show up first?

@Haehnchen do you think this could be possible?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) Security Status: Needs Decision
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
0