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

!!! TASK: Use configured baseUri in form action #100

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

simstern
Copy link
Contributor

@simstern simstern commented Sep 4, 2019

$actionRequest->getHttpRequest()->getUri() returns the original request Uri, which conflicts if the Setting Neos.Flow.http.baseUri is overwritten.

TBH: I'm not sure if this is really the best way to replace the base uri, so I'm happy for any feedback.

Closes #22

@albe
Copy link
Member

albe commented Sep 4, 2019

We're pretty much in the process of elaborating the purposes and best approach towards baseUri.
We will most likely remove the ATTRIBUTE_BASE_URI solution (which replaced previous getBaseUri() method) on the HTTP request.
See also neos/flow-development-collection#1755

/cc @bwaidelich @kitsunet

@bwaidelich
Copy link
Member

We will most likely remove the ATTRIBUTE_BASE_URI solution

Right, Christian is on it with neos/flow-development-collection#1755

Most likely the result will look like this:

$uri = $this->baseUriProvider-> getConfiguredBaseUriOrFallbackToCurrentBaseUri();

@simstern
Copy link
Contributor Author

simstern commented Sep 4, 2019

Thanks for the feedback! Christian explained me the changes regarding the baseUri this morning in the Slack channel already. I'm happy to adapt it once they are released :)

I just had to get it work for the current version anyway and therefore this PR is still done the old way for the moment.

@albe
Copy link
Member

albe commented Dec 1, 2019

Ok, so as of https://github.com/neos/flow-development-collection/pull/1755/files the final solution is indeed $uri = $this->baseUriProvider-> getConfiguredBaseUriOrFallbackToCurrentBaseUri(); with an injected instance of Neos\Flow\Http\BaseUriProvider as Bastian mentioned already

@simstern simstern force-pushed the bugfix/form-action-uri branch from 70e00b1 to 8ac6011 Compare February 9, 2022 11:15
@simstern simstern force-pushed the bugfix/form-action-uri branch from 8ac6011 to 41b1244 Compare February 9, 2022 11:17
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.

Looks good apart from the one comment

Classes/ViewHelpers/FormViewHelper.php Outdated Show resolved Hide resolved
@bwaidelich bwaidelich added the bug label Sep 13, 2022
@bwaidelich bwaidelich changed the title BUGFIX: Setting baseUri in form action uri !!! TASK: Use configured baseUri in form action Sep 13, 2022
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.

I took the freedom to make some (mostly cosmetic) tweaks.
Tested and merge this for the next major release. Thanks!!

@bwaidelich
Copy link
Member

@simstern unfortunately this breaks unit tests still and I don't have edit access to your PR

It is a good measure to allow this when creating PRs for upstream Repos:

image

@bwaidelich
Copy link
Member

FYI: We are thinking of getting rid of the notion of a global baseUri configuration (see neos/flow-development-collection#2157) and started to work on this again. So maybe it makes sense to wait for the direction this is taking before merging this one

@simstern
Copy link
Contributor Author

Thanks for your info and help on this. Makes sense to wait then.

I tried to give you edit access, but it seems I cannot change this anymore (even though, it's documented here how to do it https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork)

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

Successfully merging this pull request may close these issues.

Form does not conform to baseUri setting
3 participants