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

Add a ForceUriScheme filter to ease removal of UriNormalize filter #133

Merged
merged 1 commit into from
Apr 3, 2024

Conversation

gsteel
Copy link
Member

@gsteel gsteel commented Apr 1, 2024

Relevant to #110, #131, #132

With the planned removal of the UriNormalie filter, this new filter satisfies some of the existing functionality being removed.

@gsteel gsteel added this to the 2.36.0 milestone Apr 1, 2024
@boesing
Copy link
Member

boesing commented Apr 2, 2024

hm, I do not understand why this would be needed? Do we have some kind of real-world scenario where this become useful? It feels kinda weird that a URI-like string has to be passed just to modify the scheme afterwards.
I'd rather expect "example.org" gets "https" prepended when scheme is missing 🤔

I wonder if "parse_url" should be utilized here for the example.org but thats just an idea as I cannot see any other use-case.

@gsteel
Copy link
Member Author

gsteel commented Apr 2, 2024

The only point in this filter is to back fill some of the functionality that is lost by the removal of the UriNormalize filter. From what I can tell, UriNormalize used Laminas\Uri to do mostly the same thing, force a specific scheme, or add it if it was missing. Replacing the scheme when one already exists is pretty easy, but less so when there is none because at that point we need to start looking much more closely at whether the uri is valid or not.

To completely replace the functionality of the previous filter, We'd have to drag in another library such as league/uri or something.

I'm personally happy to just close this PR and have zero replacement for UriNormalize because I can't see a use-case beyond 'correcting' the input for http:// prefixed strings, that's why I made the default replacement https://. I don't feel like there's much value here, but theoretically people out there were using the older UriNormalize and it seems reasonable to try and meet them half way.

Either way, I'll wait for @boesing approval / more discussion before merge. The important bit is that 3.0.x can now get upgraded to SMv4

@froschdesign
Copy link
Member

It feels kinda weird that a URI-like string has to be passed just to modify the scheme afterwards.

The argument is correct because the HTML element input with type url uses properly-formed absolute URL: urlscheme://restofurl.

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/url#value

@boesing
Copy link
Member

boesing commented Apr 2, 2024

I was not aware that this re-introduces a feature which was available with the old filter.
I probably would have waited for a user to request this feature just to understand the use-case. I do not have any idea where it would be beneficial to have an whatever scheme force-converted to some pre-defined scheme 😅
But since this is now existing, I do not mind having it 🤷🏼‍♂️

@gsteel gsteel merged commit fffc9f2 into laminas:2.36.x Apr 3, 2024
14 checks passed
@gsteel gsteel deleted the force-uri-scheme-filter branch April 3, 2024 20:23
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