Explaining controllers as viable alternative #13554
Conversation
There was a problem hiding this comment.
Hi @ThomasLandauer! I've added a commit to this PR to transform it into a tip & slightly reworded the message. Please let me know if you disagree with any changes
lol I re-added the first sentence (plus an additional "instead"). We need to emphasize that this is not just some "introduction" to voters, but rather a completely different approach. And the first sentence somehow tries to explain that the following example isn't a good real-world example for the usage of voters. I thought about adding something like "for simplicity's sake", but couldn't find a good solution... |
I think it is. I wouldn't call the permission logic in this example "simple"/"basic" anymore (75+ lines of code). I don't think the first sentence adds much to the tip (it's really a separate tip) and I wanted to keep it as small as possible (long side blocks are often distracting), that's why I removed it. |
Co-Authored-By: Antoine Makdessi <antoine.makdessi@agriconomie.com>
|
What I'm trying to say: The following example is "basic" in the sense that it fulfills exactly our "new" criteria for doing it differently:
There's no reuse, and the rules are simple. So my full message would read:
Don't know how to get this across in just one short sentence ;-) |
Trying to merge the suggestions of 4 people ;-) What's really odd is that on this dedicated voters page, the reader is referred to someplace else for more info on voters ;-) > Take a look at the authorization article for an even deeper understanding on voters. So in the long run, these two should be merged - or at least the voters part integrated into this page.
security/voters.rst
Outdated
| throw $this->createAccessDeniedException(); | ||
| } | ||
|
|
||
| In that sense, the following example used throughout this page is more like a |
There was a problem hiding this comment.
I am fine with adding the explanation above, but I do not agree with this sentence. The example used below IS a real-world use case. I suggest to remove this sentence.
There was a problem hiding this comment.
I removed the second part - please take a look again :-)
|
Thank you @ThomasLandauer. |
|
@xabbuh It looks like this wasn't up-migrated to 4.4, 5.1 etc:
Should I commit it to 4.4 too? |
|
There were some conflicts when merging branches and I didn't have the time to look at them. Will probably manage to do so tomorrow. |
* 3.4: Adding `path` Fix term Change char to varchar type Use redirectToRoute [#13980] add annotation config example [Validator] Made the code sample more explicit with accepted values Update voters.rst Update voters.rst Update voters.rst Update security/voters.rst Update voters.rst [#13554] Slightly reworded the tip Update voters.rst Explaining controllers as viable alternative
* 4.4: Adding `path` Fixing bad path for class PdoSessionHandler Fix typos: DNS -> DSN Fix term Fix logout on form login setup Change char to varchar type Use redirectToRoute [#13980] add annotation config example [Validator] Made the code sample more explicit with accepted values Update voters.rst Update voters.rst Update voters.rst Update security/voters.rst Update voters.rst [#13554] Slightly reworded the tip Update voters.rst Explaining controllers as viable alternative
* 5.1: (21 commits) Adding `path` Fixing bad path for class PdoSessionHandler Fix package name modified post merge Fix typos: DNS -> DSN Fix term Fix inconsistent security docs Fix debug:container command Fix logout on form login setup Change char to varchar type Remove reference to non-existing fromBinary method Use redirectToRoute [#13980] add annotation config example [Validator] Made the code sample more explicit with accepted values Update voters.rst Update voters.rst Update voters.rst Update security/voters.rst Update voters.rst [#13554] Slightly reworded the tip Update voters.rst ...
See #13406 (comment)
Please wrap it in a "caution" box, or whatever you think is appropriate.
I'm unsure about how much context of the controller should be shown in the code, since the entire controller is only shown later. If you think it's too confusing now, I'd suggest to show the controller as "skeleton" above this note.