Use plural name from new resource metadata#999
Closed
loic425 wants to merge 1 commit intoSylius:1.13from
Closed
Conversation
loic425
force-pushed
the
use-plural-name-from-resource-metadata
branch
from
e82e959 to
899baef
Compare
loic425
force-pushed
the
use-plural-name-from-resource-metadata
branch
from
899baef to
feb8279
Compare
diimpp
approved these changes
Mar 25, 2025
Comment on lines
+137
to
+141
| if (null === $resource->getPluralName()) { | ||
| $resourcePluralName = $resourceConfiguration->getPluralName(); | ||
|
|
||
| $resource = $resource->withPluralName($resourcePluralName); | ||
| } |
Member
There was a problem hiding this comment.
Can be optimized a little.
Suggested change
| if (null === $resource->getPluralName()) { | |
| $resourcePluralName = $resourceConfiguration->getPluralName(); | |
| $resource = $resource->withPluralName($resourcePluralName); | |
| } | |
| if (null === $resource->getPluralName()) { | |
| $resource = $resource->withPluralName( | |
| $resourceConfiguration->getPluralName() | |
| ); | |
| } |
|
|
||
| $operation = $operation->withResource($resource); | ||
|
|
||
| if (null === $resource->getName()) { |
Member
There was a problem hiding this comment.
A question about line of code above this code.
$resourceConfiguration = $this->resourceRegistry->get($resource->getAlias() ?? '');We probably should assert it as Assert::notNull($resource->getAlias()); as was in some other PR/code.
| $operation = $operation->withResource($resource); | ||
| } | ||
|
|
||
| if (null === $resource->getPluralName()) { |
Member
There was a problem hiding this comment.
I guess plural name is set somehow else, so this code is removed, but I don't fully understand how or where.
Member
Author
|
This PR will need to be refactored after #984 merge. |
Member
Author
|
This has been replaced by #1108 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
It replaces #998
For now, we have two very similar metadata classes for the resource.
To calculate the route path, we use the plural name from the Metadata (the legacy one), but we can define the plural name on the ResourceMetadata.
Before this implementation, the plural name was calculated by the inflector on the MetadataClass.
With this current PR, by default, it will continue to use the plural name from the inflector (from the Medadata class), but it will use the custom one if defined. It was intended to be the case with the previous implementation, but there was an issue and the operation route factory was using the automatic plural name only.
The other plan behind is also to prepare the legacy Metadata class removal at some point.
We will be able to move the automatic plural name on the getResourceDefaults method.