-
-
Notifications
You must be signed in to change notification settings - Fork 188
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: Refactor uri helpers #3336
Conversation
This will make sure the same strategy is used in both places.
also `parseQueryIntoArguments` will be removed.
26c28aa
to
112c3e3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lovely, thanks for your efforts!
I dont know if i was to eager deleting stuff so i summon @kitsunet to please have a look at it and veto if plausible :D |
use Psr\Http\Message\UriInterface; | ||
|
||
/** | ||
* Helper to extract information from Uris. | ||
*/ | ||
abstract class UriHelper | ||
final class UriHelper |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd personally prefer the other helper, this Helper namespace seems rather pointless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah but there are already a few helpers inside: https://github.com/neos/flow-development-collection/tree/9.0/Neos.Flow/Classes/Http/Helper
and it would be a little less breaking (though I think no one uses the helper)
The other helper (from the merge yesterday) resided directly in the http namespace but at that point I was unaware of the other helpers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t exactly love the namespace either but idk it does no harm as well.
Btw you added the uri helper as part of the psr7 migration in this place ;) so better keep that location?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I guess better keep the older one, I also just realized that. Shame on me for creating that useless namespace, although back then I planned for more helpers to support using PSR-7 in Flow (but pretty much everything else exists in guzzle so no need for them). tl;dr; kk lets keep this.
See #3316
Upgrade instructions
The following methods were removed from the
UriHelper
as they are obsolete and not used.Its unlikely that the functionality is known and simple to implement yourself.
\Neos\Flow\Http\Helper\UriHelper::getUsername
\Neos\Flow\Http\Helper\UriHelper::getPassword
\Neos\Flow\Http\Helper\UriHelper::parseQueryIntoArguments
The method
\Neos\Flow\Http\Helper\UriHelper::uriWithArguments
was renamed to\Neos\Flow\Http\Helper\UriHelper::uriWithQueryParameters
to distinct between route values and query parameters which are not the same.Also it will encode the query parameters after PHP_QUERY_RFC1738.
Review instructions
The pr #3316 introduced a new uri helper while we already had one actually. This pr combines the two and cleans things up.
To ensure the logic of
uriWithAdditionalQueryParameters
is not duplicated and possibly handled elsewhere differently the helper is now also used internally by the uriConstraints.Also the method has been renamed to better fit the previous sibling
uriWithArguments
.The removed methods are dead code and previously introduced once with Flow 5.1: 8540858
As part of replacements for the old (now removed) flow Uri implementation:
Neos\Flow\Http\Uri::getUsername
->\Neos\Flow\Http\Helper\UriHelper::getUsername
Neos\Flow\Http\Uri::getPassword
->\Neos\Flow\Http\Helper\UriHelper::getPassword
Neos\Flow\Http\Uri::getArguments
->\Neos\Flow\Http\Helper\UriHelper::parseQueryIntoArguments
So maybe these methods are known in fact and it would be a bit mean to remove them just because i felt like it and we dont use / test them?
Checklist
FEATURE|TASK|BUGFIX
!!!
and have upgrade-instructions