Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Task to expand test coverage for \Drupal\Component\Utility\Url
.
Comment | File | Size | Author |
---|---|---|---|
#18 | Screen Shot 2013-10-10 at 11.21.41 PM.png | 51.7 KB | Mile23 |
#18 | Screen Shot 2013-10-10 at 11.29.59 PM.png | 50.81 KB | Mile23 |
#17 | url-phpunit-2046245-17.patch | 8.24 KB | jhedstrom |
#17 | interdiff.txt | 716 bytes | jhedstrom |
#13 | url-phpunit-2046245-13.patch | 8.06 KB | Mile23 |
Comments
Comment #1
jhedstromThis patch ups coverage to 93.8%. Currently not sure why tests for
externalIsLocal
are failing, but I've attached those as a diff, as perhaps I'm calling it incorrectly, or perhaps it is legitimately broken.Comment #2
jhedstromAdding missing phpunit tag.
Comment #3
jhedstromOops, #1 had some code from a different patch. The
failing.txt
file there is still relevant foexternalIsLocal()
.Comment #4
dawehnerWow, we are getting better and better on our test coverage!
There is a missing test for externalIsLocal() if we want to cover them all.
Comment #5
jhedstromRe: #4 see the
failing.txt
patch in #1. I'm either mis-callingexternalIsLocal()
, or it is mis-documented, or it is broken. Specifically, calling with these parameters does not return true:'http://example.com/internal/path', 'http://example.com/'
Comment #6
dawehnerYeah this function was never tested, as it just got introduced into Drupal 8.
While running these tests I get a lot of messages which looks like:
Comment #7
Mile23Added a test for when url and base_url are the same in externalIsLocal().
In the process I found a bug. :-) Actually it still had the same result but it never called line 232, so never benefitted from the short-circuit. Reading up on parse_url(), it seems the path will be '/' if the domain name has a trailing '/'.
This is one reason that 100% coverage isn't such a bad idea: You find bugs.
Comment #8
dawehnerThat is why we need this intensive unit tests!
Let's add parameter documentation / return statements.
Comment #9
Mile23The same as #7, minus the bug fix, which now has its own issue: #2076325: Drupal\Component\Utility\Url::externalIsLocal() is wrong in common cases.
Comment #10
Mile23Comment #12
Mile23OK, feel kind of stupid here. :-)
Just go back to #6, and I'll put my test additions on the other issue: #2076325: Drupal\Component\Utility\Url::externalIsLocal() is wrong in common cases.
Comment #13
Mile23Started with #6.
@param and @return added.
Removed the test for externalIsLocal(), which will be covered by the other issue.
I think @dataProvider annotation should come last in the docblock, so I put them there. No guidance (yet) from documentation standards.
Comment #14
dawehnerThe actual point in documentation is to explain what these parameters are used for, not to just list the parameters. This is information i can get by just looking at the code.
Comment #15
Mile23I think I left those alone because I didn't understand what was being tested. The rest of the docblocks look better.
Comment #16
ParisLiakos CreditAttribution: ParisLiakos commentedyes besides this small docs issue i cant see anything else
Comment #17
jhedstromFixes the docblock issues from #14.
Comment #18
Mile23CRAP 163 to 32.
Reviewed and Tested By CRAP.
Comment #19
catchCommitted/pushed to 8.x, thanks!
Comment #21
jhedstrom