-
-
Notifications
You must be signed in to change notification settings - Fork 921
Makes url generation strategy default value configurable #751
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
Makes url generation strategy default value configurable #751
Conversation
maechler
commented
Sep 12, 2016
Q | A |
---|---|
Bug fix? | no |
New feature? | yes |
BC breaks? | no |
Deprecations? | no |
Tests pass? | yes |
Fixed tickets | api-platform/api-platform#128 |
License | MIT |
@@ -44,7 +44,7 @@ public function getItemFromIri(string $iri, bool $fetchData = false); | |||
* | |||
* @return string | |||
*/ | |||
public function getIriFromItem($item, int $referenceType = UrlGeneratorInterface::ABS_PATH) : string; | |||
public function getIriFromItem($item, int $referenceType = UrlGeneratorInterface::CONFIG) : string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it needed to add this new constant? IMO everything should be set in the Configuration and Extension classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because otherwise the $referenceType parameter would be set to UrlGeneratorInterface::ABS_PATH and we would have no idea whether we should use the default config value or whether someone wanted to overwrite the default value on the method call.
Do you have another idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why cannot we let the current default value? The default value will be injected everywhere by the DIC anyway, but it will let users using components directly (without Symfony) to have sensible defaults.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this makes no sense... 😛
I have started to move the default reference type a bit up. There are really a lot of changes to be made and I have only injected the default reference type to half of the classes of which it would be necessary to. |
Indeed you can do it locally using a decorator, but IMO it would be a very nice improvement to be able to configure that globally in core. |
@dunglas Would you be ok with the way I injected the configuration in all classes that need it? If you have better ideas to implement this let me know. |
@maechler sorry for being so long to reply. I need to review this more deeply. I think it is possible to pass the constant to use in the function parameter in most case instead of adding global default values as states. |
@dunglas Never mind! That would be fine too of course. Anyways..you guys are doing a great job with the development of api-platform! Thanks! |
@antograssiot No worries! It is good to see that this issue is being revived, thanks. |