Add configurable naming strategy #547
Conversation
|
We have to be clear what this naming strategy is for, and |
|
naming strategy is just about naming routes?, and what about |
|
I don't think we should restrict it to just naming |
|
Are there others usages you think of ? For naming, I have not much idea.. Its a method transforming a camel case name to an url segment so transformResourceNameToUrl() maybe ? |
|
I suggest this method signature: public function namePath(string $resourceShortName) : string; |
Not at the moment. Actually, I take back what I said. It's better to have smaller interfaces that only do one thing. We could make this |
|
I've updated with your comments |
| { | ||
| private $pathNamer; | ||
|
|
||
| public function __construct( |
There was a problem hiding this comment.
This should be kept as one line (Symfony CS).
|
I've updated with your comments |
| $resourceShortName = $resourceMetadata->getShortName(); | ||
|
|
||
| if (null === $resourceShortName) { | ||
| throw new InvalidResourceException(sprintf('Resource "%s" have a null short name.', $resourceClass)); |
There was a problem hiding this comment.
"Resource %s has no short name defined."
|
Done |
|
LGTM 👍 rebase needed though |
|
👍 |
|
Needs rebase. ping @api-platform/core-team |
| */ | ||
| public function testNoShortNameApiLoader() | ||
| { | ||
| $resourceMetadata = new ResourceMetadata(); |
There was a problem hiding this comment.
What do you mean?
$this->getApiLoaderWithResourceMetadata(new ResourceMetadata())->load(null); ?
There was a problem hiding this comment.
Why should it be inlined? It will be less readable...
There was a problem hiding this comment.
Let me be more precise. The variable assignation is useless. It can be on 2 lines for readability, but without the assignation :)
|
👍 |
|
Updated I think it good now :) |
|
Thank you @polc! Can you open a doc PR now please? https://github.com/api-platform/doc |
|
@dunglas Sure |
Add configurable naming strategy
So here is a simple naming strategy PR. I couldn't check if the tests pass because I have a weird issue on my computer so wait until I resolve all test
Some things I'm not sure:
If this PR is merged, I would like to change the default strategy (in an other PR) to dashs strategy maybe