Skip to content

[FEATURE] Make relative, absolute and links relative to a base URL possible #614

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

Merged
merged 5 commits into from
Oct 24, 2023

Conversation

linawolf
Copy link
Contributor

@linawolf linawolf commented Oct 3, 2023

depends on #607

Copy link
Member

@jaapio jaapio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this massive improvement!

In general this looks very good, however I would like to see an improvement that makes the UrlGenerator a bit more light weight. And that might reduce the number of changes you need to do.

Right now the UrlGenerator is basically handling 2 different strategies:

  1. Generate urls relative
  2. Generate urls absolute

If we would split this into 2 different classes, we could reduce the complexity of the UrlGenerator and also make it easier to understand.
Beside that you could replace the new UrlGenerator in the RenderContext creation with an
injected UrlGeneratorInterface. This would make it easier to replace the UrlGenerator with a custom implementation.

This way the RenderContext does not require the settingsManager anymore. And we could
even inject the UrlGenerator everywhere we need it. To accomplish this
you could set the correct UrlGenerator in the GuidesExtension::load method, like you
do with the ThemeManager.

Another benefit of this approach is that we could inverse the dependency on the UrlGenerator
and RenderContext. This way the UrlGenerator can take what ever it needs from the RenderContext.
For example:

   $urlGenerator->generateUrl($renderContext, $cannonicalUrl);

As an extra, you could allow people to configure the UrlGeneator in the Guides.xml.

<guides>
    <urlGenerator class="My\Custom\UrlGenerator" />
</guides>

Or for buildin strategies:

<guides>
    <urlGenerator strategy="relative" />
</guides>

@linawolf linawolf force-pushed the task/renderAbsolute-Or-Relative-Urls branch from 19cfb9c to 65cfda7 Compare October 4, 2023 13:53
@linawolf linawolf force-pushed the task/renderAbsolute-Or-Relative-Urls branch from 65cfda7 to d366144 Compare October 21, 2023 09:16
@linawolf linawolf marked this pull request as draft October 21, 2023 09:19
@linawolf linawolf force-pushed the task/renderAbsolute-Or-Relative-Urls branch 7 times, most recently from aa1081d to ff52f80 Compare October 22, 2023 05:55
@linawolf linawolf marked this pull request as ready for review October 22, 2023 05:59
Copy link
Member

@jaapio jaapio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a single question. This looks good to go for me.

Thanks!

@linawolf linawolf force-pushed the task/renderAbsolute-Or-Relative-Urls branch from ff52f80 to 2dc6366 Compare October 24, 2023 15:22
@linawolf linawolf force-pushed the task/renderAbsolute-Or-Relative-Urls branch from 2dc6366 to e39a1d5 Compare October 24, 2023 16:15
@linawolf linawolf merged commit 3fa34a4 into main Oct 24, 2023
@linawolf linawolf deleted the task/renderAbsolute-Or-Relative-Urls branch October 24, 2023 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants