Skip to content

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

Closed
wants to merge 1 commit into from
Closed

Makes url generation strategy default value configurable #751

wants to merge 1 commit into from

Conversation

maechler
Copy link
Contributor

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;
Copy link
Member

@dunglas dunglas Sep 12, 2016

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.

Copy link
Contributor Author

@maechler maechler Sep 12, 2016

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?

Copy link
Member

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.

Copy link
Contributor

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... 😛

@maechler maechler mentioned this pull request Sep 12, 2016
5 tasks
@maechler
Copy link
Contributor Author

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.
I do not think this is becoming a much cleaner solution. I am not sure if it makes sense to include this feature. What do you think?
For our use case it would actually be enough to just decorate the UrlGenerator and force everything to be absolute URLs.

@dunglas
Copy link
Member

dunglas commented Sep 25, 2016

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.

@maechler
Copy link
Contributor Author

@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.

@dunglas
Copy link
Member

dunglas commented Oct 12, 2016

@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.

@maechler
Copy link
Contributor Author

@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
Copy link
Contributor

Thanks for @maechler, I've restarted this in #2708
Unfortunately I was not able to keep your commit as the original repository was closed.

@maechler
Copy link
Contributor Author

maechler commented Apr 8, 2019

@antograssiot No worries! It is good to see that this issue is being revived, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants