Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhedstrom’s picture

Status: Active » Needs review
FileSize
8.4 KB
1.18 KB

This 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.

jhedstrom’s picture

Issue tags: +PHPUnit

Adding missing phpunit tag.

jhedstrom’s picture

Oops, #1 had some code from a different patch. The failing.txt file there is still relevant fo externalIsLocal().

dawehner’s picture

Wow, we are getting better and better on our test coverage!

There is a missing test for externalIsLocal() if we want to cover them all.

jhedstrom’s picture

Re: #4 see the failing.txt patch in #1. I'm either mis-calling externalIsLocal(), 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/'

dawehner’s picture

Yeah 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:

Empty test suite.
Empty test suite.
Empty test suite.
Empty test suite.
Empty test suite.
Empty test suite.
Empty test suite.
Empty test suite.
Empty test suite.
Empty test suite.
Empty test suite.

Mile23’s picture

Added 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.

dawehner’s picture

Status: Needs review » Needs work

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 '/'.

That is why we need this intensive unit tests!

+++ b/core/tests/Drupal/Tests/Component/Utility/UrlTest.php
@@ -189,6 +190,185 @@ public function testInvalidRelative($url, $prefix) {
+  public function testFilterQueryParameters($query, $exclude, $expected) {
...
+   * Provides data to self::testFilterQueryParameters().
+   */
+  public static function providerTestFilterQueryParameters() {
...
+  public function testParse($url, $expected) {
...
+  public static function providerTestParse() {
...
+  public function testEncodePath($path, $expected) {
...
+  public static function providerTestEncodePath() {
...
+  public function testIsExternal($path, $expected) {
...
+   */
+  public static function providerTestIsExternal() {
...
+  public function testFilterBadProtocol($uri, $expected, $protocols) {
...
+   */
+  public static function providerTestFilterBadProtocol() {
...
+   */
+  public function testStripDangerousProtocols($uri, $expected, $protocols) {
...
+   */
+  public static function providerTestStripDangerousProtocols() {

@@ -228,4 +408,29 @@ protected function dataEnhanceWithPrefix(array $urls) {
+   */
+  public function providerTestExternalIsLocal() {
...
+   */
+  public function testExternalIsLocal($url, $base_url, $expected) {

Let's add parameter documentation / return statements.

Mile23’s picture

FileSize
7.15 KB

The same as #7, minus the bug fix, which now has its own issue: #2076325: Drupal\Component\Utility\Url::externalIsLocal() is wrong in common cases.

Mile23’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, url-phpunit-2046245-9.patch, failed testing.

Mile23’s picture

OK, 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.

Mile23’s picture

Status: Needs work » Needs review
FileSize
8.06 KB

Started 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.

dawehner’s picture

+++ b/core/tests/Drupal/Tests/Component/Utility/UrlTest.php
@@ -189,6 +198,230 @@ public function testInvalidRelative($url, $prefix) {
+   * @param array $query
+   * @param array $exclude
+   * @param array $expected

The 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.

Mile23’s picture

I think I left those alone because I didn't understand what was being tested. The rest of the docblocks look better.

ParisLiakos’s picture

Status: Needs review » Needs work

yes besides this small docs issue i cant see anything else

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
716 bytes
8.24 KB

Fixes the docblock issues from #14.

Mile23’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
50.81 KB
51.7 KB

CRAP 163 to 32.

Reviewed and Tested By CRAP.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.

jhedstrom’s picture