\Drupal\Core\Url has a toString() method, but not __toString(). I think we should implement __toString() so that we can just leave off ->toString() or use (string) casting as necessary.

Files: 
CommentFileSizeAuthor
#22 2350507-22.patch1.26 KBhussainweb
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,041 pass(es). View
#22 interdiff-11-22.txt529 byteshussainweb
#11 2350507-11.patch1.21 KBhctom
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,146 pass(es), 3 fail(s), and 0 exception(s). View

Comments

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Amsterdam2014

RTBC - most useful for twig templates obviously!

I don't think we need test coverage as ->toString() already exists and should be tested, so this would be testing PHP ...

dawehner’s picture

Status: Reviewed & tested by the community » Needs review

We explicit did not wanted to add it. This needs more discussion in case we really wanna support it.

In general I prefer explicit handling with these objects as otherwise we can easily create bug reports.

Crell’s picture

FTR I would support adding the magic method in addition toString().

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Twig

This is for Twig, so needed as we want to have magic methods on everything we want to print.

This is only in addition to the explicit method, so should never hurt normal usage.

webchick’s picture

Assigned: Unassigned » dawehner
Status: Reviewed & tested by the community » Needs review

dawehner, what do you think?

larowlan’s picture

+1 to both

jbrown’s picture

Why does there need to be an explicit method? Isn't it better to cast it as a string?

dawehner’s picture

Fabianx’s picture

Issue tags: +Novice, +Needs patch

Tagging novice and needs patch to move the privatepaste with comments fixed into a patch

hctom’s picture

I'll work on this...

hctom’s picture

FileSize
1.21 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,146 pass(es), 3 fail(s), and 0 exception(s). View
668 bytes

Attached you can find the new patch based on the privatepaste stuff, which unfortunately was corrupt, but I managed it manually. Unfortunately the testMagicToString() test breaks with the following message:

Drupal\Tests\Core\UrlTest::testMagicToString Method Drupal\Core\Url::__toString() must return a string value

I tried to figure out what happens, but I did not get any clue, what's wrong... The strange thing is, that the test almost does the same as testToString(). When I comment the $this->assertSame($path, $url->toString());line in the testToString() method, the testMagicToString()passes... so I think there might be a problem with the Url->toString() method itself?!

hctom’s picture

Status: Needs review » Needs work

Back to Needs work

penyaskito’s picture

AFAIK, you should do:

+  public function __toString() {
+    return (string) $this->toString();
+  }

The last submitted patch, 11: 2350507-11.patch, failed testing.

Fabianx’s picture

#13 Makes sense.

hctom’s picture

I don't think #13 makes sense, as this only solves the potential issue for the magic method, doesn't it? I am really confused why it should be necessary to cast a toString methods as a string - which obviously should be obsolete, doesn't it? In my opinion the toString method itself should ensure, that a string is returned (which is implied by its function name and both called methods inside return strings, as their docblocks say).

Or am I missing something here?

jbrown’s picture

@hctom you are right - #13 does not make sense. #11 is correct.

jbrown’s picture

hussainweb’s picture

Status: Needs work » Needs review
Issue tags: -Needs patch
FileSize
1.44 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,440 pass(es), 1 fail(s), and 0 exception(s). View
822 bytes

If my analysis is correct, the problem is using array_pop in the test methods. Let's see if this patch works.

hussainweb’s picture

I realized that setup is called for each test, so array_pop shouldn't be a problem. I will follow the line of thought in #11 to see if I can find something.

Status: Needs review » Needs work

The last submitted patch, 19: url-tostring-2350507-19.patch, failed testing.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
529 bytes
1.26 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,041 pass(es). View

I am ignoring changes in #19. The interdiff is against the patch in #11.

I spent a good many hours on this, but it was fruitful. I learnt a lot about PHPUnit. The problem was that the UrlGenerator is mocked in setUp before every test method, but the urls themselves are generated once in testUrlFromRequest and used in all other test methods (that depend on it). This means that the urlGenerator gets cached in each Url object in the first test method run, and in the subsequent runs, when the mocked object has been destroyed (or whatever), references in those Url objects still point there. Attempting to call methods on that mock always returns NULL, which is not a string like the error message says.

The proper fix needs a little refactoring and I should open a followup for that. For now, I have just called setUrlGenerator in the test method like some other methods. I am just putting it up for review but I think that at the very least, there should be a comment explaining why we are calling that.

hctom’s picture

Yeah... nice one ;) But just in case, shouldn't this $url->setUrlGenerator(\Drupal::urlGenerator()); part also be applied to the "normal" testToString method? Or isn't it necessary at this point?

hussainweb’s picture

It should be, but only for the sake of consistency. Right now, that is the first test method called which is dependant on the testUrlFromRequest and at that point, the urlGenerator is not set on the object yet, which sets it to the mocked one.

I think we should instead plan to refactor this properly. We are just leaving it open for more problems. The fixture is not consistent between tests and I think it is important it should be. We could do this on a follow-up issue. I am curious to see what everyone thinks.

hctom’s picture

I just took a deeper look into the whole UrlTest class and I think hussainweb is completely right. There is a whole lot of refactoring work to do before implementing our tests. And in addition to that I found the \Drupal\Tests\Core\UnroutedUrlTest class, which we completely ignored so far ;)

From my understanding there are a lot of "wrong" test implementations. E.g. all test methods that list @depends testUrlFromRequest should use a @dataProvider annotation instead and the testUrlFromRequest() method could use the @dataProvider too. With this, we could get rid of the $this->map property to create dummy URL information and use the dataProvider approach, PHPUnit offers.

With this refactoring we should be safe to no object/property changes inbetween different test methods, but I guess this is no follow-up issue, it is more of a "must be done before we can go on" issue ;) Otherwise we won't be sure that no other test method breaks another one.

So here is the issue I just created: #2358709: Refactor UnroutedUrlTest and UrlTest tests

hussainweb’s picture

There is a problem with using @dataProvider. I had started with that but then gave it up because that also needs refactoring. Thanks for creating the follow-up issue. :)

hctom’s picture

@hussainweb: What kind of problem?

hussainweb’s picture

I am in a little bit of hurry right now, or I would explain. It is about the order of functions executed. Data providers are run before setUp which means we need to refactor to be able to use it. I will explain in more detail with the fixes as I see it later. Thanks!

hctom’s picture

@hussainweb: Okay... perhaps you can have a look at my patch over at #2358709: Refactor UnroutedUrlTest and UrlTest tests?! It still has an issue with the test of the Url::toString() method, but I am not sure, if this is my test's problem or a problem within the method itself.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

This is great, makes it much easier to use this URL object in a twig template!

hctom’s picture

Status: Reviewed & tested by the community » Needs work

Please read my comment in #29 - there are still issues with it, which should be fixed before.

Fabianx’s picture

Status: Needs work » Reviewed & tested by the community

I think the follow-up issue is fine.

This issue is about the __toString method and that is tested well enough. If the original toString method has problems, we can address that in the follow-up, but this blocks themers from using urls as intended, so would be nice to get in.

hctom’s picture

Okay... I just thought we should get rid of the adressed problems BEFORE producing new issues, that are already mentioned here...

joelpittet’s picture

Assigned: dawehner » Unassigned

@hctom thanks for keeping tabs, yes I agree we should create a followup to clean up the tests. If there are issues with the methods or tests in this patch, let's fix those specifically here.

I don't claim to be an expert on tests by any means, but this one seems fairly straight forward and does what is necessary for this issue's summary functionally.

Do we still need @dataProvider as you mentioned above? Could you roll a patch with the fixes needed for testMagicToString documentation docblock?

hctom’s picture

This is what I am trying to say all the time: There is a major rewrite of the tests needed, which I already did in #2358709: Refactor UnroutedUrlTest and UrlTest tests.

The only problem there is, that the toString() still throws errors, which in my opinion shows, that there is something wrong with it! So just have a look at the "follow up" issue and when that is done we can reimplement the testMagickToString() method into the test... And of course: Implement it in UnroutedUrl class, too.

Fabianx’s picture

Yes, but the problem is in the normal toString() method and not in the magic __toString() method, which is added here, which is why it is fine to add.

The follow-up is well warranted, but it is for existing problematic code, not new code.

Or in other words, once toString() works properly and because __toString() calls toString(), __toString() will work fine, too per deductive logic.

So there is no need to delay adding __toString if that is just a consumer of that method - regardless of in which state it is in.

hctom’s picture

Okay... nevermind - I would not implement something that obviously makes problems and throws exceptions, but this is up to you... and we won't have to discuss here, if someone of you would have taken a look at my follow up in the meantime ;) So again, here is an action call to also check and rework/review #2358709: Refactor UnroutedUrlTest and UrlTest tests!!

dawehner’s picture

+1 given that we want to use it in templates.

+++ b/core/lib/Drupal/Core/Url.php
@@ -452,6 +452,13 @@ public function toString() {
+   * Implements __toString magic method for conversion into a string.

Please use {@inheritdoc} ... this line of documentation does not give any additional information.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per #2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase?. Committed 338c791 and pushed to 8.0.x. Thanks!

diff --git a/core/lib/Drupal/Core/Url.php b/core/lib/Drupal/Core/Url.php
index 8c7b0df..9ea38b2 100644
--- a/core/lib/Drupal/Core/Url.php
+++ b/core/lib/Drupal/Core/Url.php
@@ -452,7 +452,7 @@ public function toString() {
   }
 
   /**
-   * Implements __toString magic method for conversion into a string.
+   * {@inheritdoc}
    */
   public function __toString() {
     return $this->toString();

Fixed on commit.

  • alexpott committed 338c791 on 8.0.x
    Issue #2350507 by hussainweb, hctom, jbrown: Fixed \Drupal\Core\Url has...

Status: Fixed » Closed (fixed)

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

BR0kEN’s picture

Status: Closed (fixed) » Needs work

Why this is not in 8.1.x+?

dawehner’s picture

Status: Needs work » Closed (fixed)

Because it was reverted as part of #2416971: Remove Url::__toString()

BR0kEN’s picture

@dawehner, removed instead of returning an empty string and triggering user error? Hm...