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

BUGFIX: Fix UriConstraints.applyTo() port handling #2676

Merged
merged 1 commit into from
Jan 20, 2022

Conversation

kdambekalns
Copy link
Member

No description provided.

Copy link
Member

@bwaidelich bwaidelich left a comment

Choose a reason for hiding this comment

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

beautiful! :)

@@ -358,7 +358,7 @@ public function applyTo(UriInterface $baseUri, bool $forceAbsoluteUri): UriInter
$uri = $uri->withHost($host);
}
}
if (isset($this->constraints[self::CONSTRAINT_PORT]) && ($forceAbsoluteUri || $this->constraints[self::CONSTRAINT_PORT] !== $baseUri->getPort()) && ($baseUri->getPort() !== null || $this->constraints[self::CONSTRAINT_PORT] !== UriHelper::getDefaultPortForScheme($baseUri->getScheme()))) {
if (isset($this->constraints[self::CONSTRAINT_PORT]) && ($forceAbsoluteUri || $this->constraints[self::CONSTRAINT_PORT] !== $baseUri->getPort()) && ($baseUri->getPort() !== null || $this->constraints[self::CONSTRAINT_PORT] !== UriHelper::getDefaultPortForScheme($this->constraints[self::CONSTRAINT_SCHEME] ?? $baseUri->getScheme()))) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (isset($this->constraints[self::CONSTRAINT_PORT]) && ($forceAbsoluteUri || $this->constraints[self::CONSTRAINT_PORT] !== $baseUri->getPort()) && ($baseUri->getPort() !== null || $this->constraints[self::CONSTRAINT_PORT] !== UriHelper::getDefaultPortForScheme($this->constraints[self::CONSTRAINT_SCHEME] ?? $baseUri->getScheme()))) {
if (isset($this->constraints[self::CONSTRAINT_PORT]) &&
($forceAbsoluteUri || $this->constraints[self::CONSTRAINT_PORT] !== $baseUri->getPort()) &&
($baseUri->getPort() !== null || $this->constraints[self::CONSTRAINT_PORT] !== UriHelper::getDefaultPortForScheme($uri->getScheme()))) {

Since the scheme constraint is already applied to the new URI this would be more straightforward maybe and be analogous to https://github.com/neos/flow-development-collection/pull/2676/files#diff-8df45416161efdbe2035f01361dc8245e81a126fc892f1497ba972acb07738bcR407?
I see it as "apply constraints to the new URI in a specific order and depending on previous applied constraints"

Copy link
Member

Choose a reason for hiding this comment

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

Since the scheme constraint is already applied to the new URI

Only if the scheme constraint does not match the scheme of the base URI

Copy link
Member Author

Choose a reason for hiding this comment

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

It's only applied if different, so this would not be working here. Tried…

Copy link
Member

@albe albe left a comment

Choose a reason for hiding this comment

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

Tests pass again, thanks! Left a tiny suggestion but not urgent

@kdambekalns kdambekalns merged commit 171b029 into neos:7.0 Jan 20, 2022
@kdambekalns kdambekalns deleted the bugfix/uriconstraints-applyto branch January 20, 2022 11:53
@markusguenther
Copy link
Member

Thanks @kdambekalns, for dive deeper and fix that issue :)

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.

4 participants