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: Refactor uri helpers #3336

Merged
merged 6 commits into from
Mar 29, 2024
Merged

Conversation

mhsdesign
Copy link
Member

@mhsdesign mhsdesign commented Mar 28, 2024

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

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the lowest maintained branch
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

@mhsdesign mhsdesign force-pushed the task/3316-followup-merge-uri-helpers branch from 26c28aa to 112c3e3 Compare March 28, 2024 20:43
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.

Lovely, thanks for your efforts!

@mhsdesign
Copy link
Member Author

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
Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member Author

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?

Copy link
Member

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.

@kitsunet kitsunet merged commit 408854c into 9.0 Mar 29, 2024
9 checks passed
@kitsunet kitsunet deleted the task/3316-followup-merge-uri-helpers branch March 29, 2024 09:05
@mhsdesign mhsdesign changed the title TASK: 3316 followup merge uri helpers !!! TASK: Refactor uri helpers Mar 29, 2024
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.

3 participants