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

fix #17191: improved consistency and readability of UrlManager::creat… #20089

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

Conversation

ggh2e3
Copy link
Contributor

@ggh2e3 ggh2e3 commented Dec 24, 2023

Q A
Is bugfix? ✔️
New feature?
Breaks BC?
Fixed issues #17191

Hi, in order to improve readability and make the behaviour more consistent, I have changed the implementation of both createUrl and CreateAbsoluteUrl methods. They're now based on Url::isRelative method. I think there are (already) enough tests to cover those cases.

Thank you :)

Copy link

what-the-diff bot commented Dec 24, 2023

PR Summary

  • Bug Fix in URL Management
    The team has rectified an issue present in the method used for creating URL parameters. This was achieved by modifying the logic to depend on another method called 'isRelative'. This should enhance URL generation and routing processes.

  • Improved URL Handling Method
    The 'isRelative' method of the system's URL handling tool has been debugged to better distinguish between relative and absolute URLs.

  • Enhancement in Link Serialization
    We've revised a method that organizes or 'serializes' web links in the system's Web Link management file, correcting the issues that previously existed. This should increase the efficiency and effectiveness of web link manipulation.

  • MSSQL Boolean Type Bug Rectified
    The team corrected a problem related to Boolean type data handling in the MSSQL file. This will ensure the appropriate management of Boolean variables in the system's database interactions.

Copy link

codecov bot commented Dec 24, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (dffb4c7) 48.02% compared to head (9cece00) 48.02%.

Files Patch % Lines
framework/web/UrlManager.php 84.61% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #20089      +/-   ##
==========================================
- Coverage   48.02%   48.02%   -0.01%     
==========================================
  Files         445      445              
  Lines       43890    43884       -6     
==========================================
- Hits        21078    21074       -4     
+ Misses      22812    22810       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@samdark samdark requested review from a team December 25, 2023 09:56
@samdark
Copy link
Member

samdark commented Dec 25, 2023

@SamMousa, @rob006 want to take a look?

@rob006
Copy link
Contributor

rob006 commented Dec 27, 2023

I'm not sure about this. After #20077 Url::isRelative() becomes significantly slower (BTW: I think it would better to ensure that we search for :// only before ? instead of relying on parse_url()) and these functions could be called hundreds of times in single request.

Also, new code does not seem to be equivalent to the old one, so I would like to see some explanation why it was changed.

@ggh2e3
Copy link
Contributor Author

ggh2e3 commented Dec 29, 2023

I was trying to fix #17191 @rob006. Reading throughout the message I came across this #17191 (comment). After fixing isRelative in #20077, I modified both createUrl and createAbsoluteUrl to make the behaviour more consistent. Why do you say that isRelative is slower now? Moreover, the code behaviour is the same as before, I would expect tests to fail otherwise.

@SamMousa
Copy link
Contributor

BTW: I think it would better to ensure that we search for :// only before ? instead of relying on parse_url()) and these functions could be called hundreds of times in single request.

I'm skeptical on this. I'd expect functions natively in php to be faster. Regardless, parse_url has this note:

Caution
This function may not give correct results for relative or invalid URLs, and the results may not even match common behavior of HTTP clients. If URLs from untrusted input need to be parsed, extra validation is required, e.g. by using [filter_var()](https://www.php.net/manual/en/function.filter-var.php) with the FILTER_VALIDATE_URL filter.

@SamMousa
Copy link
Contributor

SamMousa commented Dec 29, 2023

@ggh2e3 can you give some example of inconsistencies in the old code?
Because while they might be considered bugs they could also be behavior people depend on.

@rob006
Copy link
Contributor

rob006 commented Dec 29, 2023

@ggh2e3 @SamMousa parse_url() is quite complex and it may be an overkill in this case. I made some tests before and two strpos() calls were 20 times faster than single parse_url().

Also, I have very little faith in tests in this case. It really looks like tests cover only basic scenarios and I'm sure there are edge cases not covered by tests.

@ggh2e3
Copy link
Contributor Author

ggh2e3 commented Dec 30, 2023

@SamMousa when I fixed isRelative, I wrote some tests. The only one failing with the old implementation was the one reported in #17191. You'll find the use case here:
tests/framework/helpers/BaseUrlTest.php:79

@rob006 if you provide some examples (or tell me where I can find some) I may improve the test coverage

@SamMousa
Copy link
Contributor

I don't see changes to the Base URL class in this PR though.. am I missing something? (Sorry on mobile so not ideal for browsing gh)

@rob006
Copy link
Contributor

rob006 commented Dec 31, 2023

@ggh2e3 I don't have any particular case in mind, but for example you PR removed ($pos = strpos($url, '/', 2)) !== false case. I'm not sure what is purpose of it, but it looks like added on purpose, so messing with such old and crucial code in this way is pretty risky.

@yiisoft yiisoft deleted a comment from Webkadabra Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants