<front> is not implemented correctly [#2538928]

Comment	File	Size	Author
#5	front_is_not-2538928-5-interdiff.txt	954 bytes	mbovan
#5	front_is_not-2538928-5.patch	1.22 KB	mbovan

#3	front_is_not-2538928-3-TEST-ONLY.patch	555 bytes	mbovan

#1	front_is_not-2538928-1.patch	1.2 KB	mbovan

Comment #1

mbovan CreditAttribution: mbovan at MD Systems GmbH commented 24 July 2015 at 10:44

Status:

Active

» Needs review

File	Size
front_is_not-2538928-1.patch	1.2 KB

Providing a fix.

Log in or register to post comments

Comment #2

Berdir

German

Switzerland

CreditAttribution: Berdir at MD Systems GmbH commented 24 July 2015 at 19:05

Status:

Needs review

» Needs work

+++ b/src/Tests/PathologicTest.php
@@ -141,7 +141,7 @@ class PathologicTest extends WebTestBase {
         check_markup('<a href="/"></a>', $format_id),
-        '<a href="' . $paths['/'] . '"></a>',
+        '<a href="' . str_replace('%3Cfront%3E', '', $paths['/']) . '"></a>',
         t('Path with just slash. Clean URLs: !clean; protocol style: !ps', $t10ns)

I don't really get what this is doing :)

Can we have a test-fail patch?

Log in or register to post comments

Comment #3

mbovan CreditAttribution: mbovan at MD Systems GmbH commented 27 July 2015 at 10:33

Status:

Needs work

» Needs review

File	Size
front_is_not-2538928-3-TEST-ONLY.patch	555 bytes

Here is the test-only patch.

Re #2: $paths['/'] contains something like: "http//d8.dev/"... To assert results correctly I replaced <front> (encoded as %3Cfront%3E) with empty string.

Log in or register to post comments

Comment #4

27 July 2015 at 10:35

Status:

Needs review

» Needs work

The last submitted patch, 3: front_is_not-2538928-3-TEST-ONLY.patch, failed testing.

Log in or register to post comments

Comment #5

mbovan CreditAttribution: mbovan at MD Systems GmbH commented 27 July 2015 at 11:36

Status:

Needs work

» Needs review

File	Size
front_is_not-2538928-5.patch	1.22 KB

front_is_not-2538928-5-interdiff.txt	954 bytes

As discussed, changed test method that returns $path, instead of changing $path output in assertEqual() .

Log in or register to post comments

Comment #6

Berdir

German

Switzerland

CreditAttribution: Berdir at MD Systems GmbH commented 27 July 2015 at 11:49

Status:

Needs review

» Reviewed & tested by the community

I guess that check could be outside of the if but yes, I like this a lot more. Before, we were testing for the broken behavior because both places did it incorrectly.

Log in or register to post comments

Comment #8

Garrett Albright CreditAttribution: Garrett Albright commented 29 July 2015 at 04:40

Weird. Tests don't fail before this patch… it looks like the output of the test case was wrong in the same way that Pathologic was wrong. Not really sure what to make of that.

Anyway, committed and pushed.

Log in or register to post comments

Comment #9

Garrett Albright CreditAttribution: Garrett Albright commented 29 July 2015 at 04:40

Version:	8.x-1.x-dev	» 7.x-3.x-dev
Status:	Reviewed & tested by the community	» Needs review

Marking for NR on the D7 branch so we can see if it's having the same problem. I don't think it does, though.

Log in or register to post comments

Comment #10

Berdir

German

Switzerland

CreditAttribution: Berdir at MD Systems GmbH commented 29 July 2015 at 07:06

Version:	7.x-3.x-dev	» 8.x-1.x-dev
Status:	Needs review	» Fixed

Yes, 7.x-1.x is certainly fine, this is completely different in 8.x, moving back to 8.x :) And yes, that's the problem with dynamic assertions, when the test is implementing the same logic as the actual code then you aren't testing much :)

Not easy to change that for URL's, only thing I can see is having hardcoded Url::fromRoute('') and so on objects that you are comparing against, that might be a bit clearer than a helper function that does magic.