Skip to content
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

Allow to configure hostname for redirect #53

Merged
merged 8 commits into from
Jan 15, 2021
Merged

Allow to configure hostname for redirect #53

merged 8 commits into from
Jan 15, 2021

Conversation

choonge
Copy link
Contributor

@choonge choonge commented Dec 14, 2020

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Fixed tickets fixes #issuenum
Related issues/PRs #issuenum
License MIT

What's in this PR?

Implementation of #35

Why?

Allows for redirects by per hostname. Hostname is optional; redirects will fall back to the one without defined sourceHost if no specific redirect exists.

Example Usage

  1. Add redirects in Sulu admin for different hostnames
  2. Verify that the redirects kick in for the different hostnames, even if the source URL is the same

@niklasnatter niklasnatter deleted the branch sulu:0.x January 12, 2021 12:53
@niklasnatter niklasnatter reopened this Jan 12, 2021
@niklasnatter niklasnatter changed the base branch from master to 0.x January 12, 2021 12:56
Copy link
Contributor

@niklasnatter niklasnatter left a comment

Choose a reason for hiding this comment

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

Hey,
I am sorry for not reacting on this sooner. I did not find the time to have a deeper look on this until now. I hope the delay did not cause any big inconvenience for you.

We have planned to not add any new features to the Sulu 1 branches of our projects after the release of Sulu 2. But I will talk to the team after the comments are resolved to make an exception here because the code looks well built and we have not tagged an official Sulu 2 version of this bundle.

Also, we found some time to move the tests of the bundle to Github actions. Would be nice if you could rebase your branch onto the 0.x branch to run the pipelines.

Thanks a lot for your effort and sorry again for the delay!

Entity/RedirectRoute.php Outdated Show resolved Hide resolved
Model/RedirectRouteInterface.php Outdated Show resolved Hide resolved
Model/RedirectRouteInterface.php Outdated Show resolved Hide resolved
Routing/RedirectRouteProvider.php Outdated Show resolved Hide resolved
UPGRADING.md Outdated Show resolved Hide resolved
Manager/RedirectRouteNotUniqueException.php Show resolved Hide resolved
@choonge
Copy link
Contributor Author

choonge commented Jan 14, 2021

Hi,

No problem at all; we've forked this repo. But it would be neat if the change could make it into the official repo, so we have a bit less maintenance overhead in the future. On the other hand it's not a big problem if you would decide not to merge it after all.

In any case, thanks a lot for taking the time to do the PR review! Your comments have been processed and the branch has been rebased onto 0.x.

Please let me know if you spot any other improvements. Thanks again!

@niklasnatter niklasnatter changed the title Allow to configure redirects by hostname Allow to configure hostname for redirect Jan 15, 2021
@niklasnatter
Copy link
Contributor

@choonge The team decided to merge this as we wanted to include this feature in the first Sulu 2 version of the bundle anyway. Sorry again for the delay - thanks a lot for working on this 🙂

@choonge choonge deleted the hotfix/redirect-by-host branch January 18, 2021 07:09
@choonge
Copy link
Contributor Author

choonge commented Jan 18, 2021

@nnatter Great news, thank you! When ready, could you please create a new tag? Thanks again!

@niklasnatter
Copy link
Contributor

We would like to merge #34 and #36 before creating a tag. But after that, we will tag a final Sulu 1 release 🙂

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.

3 participants