Add proper support for Url objects in FieldPluginBase::renderAsLink(), so we can remove EntityInterface::getSystemPath()

I hope this clarification is correct?

Re-roll.

If #2410499-3: Remove remaining _l() calls in Views is true, then this blocks #2343669: Remove _l() and _url(), so escalating to critical, and blocker.

Probably still needs some work...

Real patch. Probably still needs some work...

Working on replumbing w/ URL objects.

Work is progress to see what TestBot says.

Fixed some routes names and link template names.

This is likely blocked on #2418139: Add a toUriString method to Url class and add a route: scheme. The problem has to do with needing the ability to do token replacement on paths, and currently ending up with the base getting added twice. So, we need the ability to token replace on the URI before we convert it to a URL.

I am still working on this, though, so keeping it assigned to myself.

Blocker is in

Just tidying up some metadata.

That night the children slept, whilst dreams of diffs danced through their head.
Would the patch fairy visit them in the night?

kicking it further along, down to 4 fails in the unit test.

really bizarre how we create a url object more than once in the ::renderLink method - I think there is real scope to simplify there once we have passing unit-tests. Feels especially weird that we keep dropping it back to a string path and re-parse that instead of sticking to the Url object.

2hrs plus of staring at this is enough, more tomorrow.

more stuff
we're going string -> url ->string -> url too much

Try to find me on IRC today. I was still working on that. We had passing unit tests on this before the last round of work on it.

Assigning myself. Will work on it, update issue summary, etc, with what we discussed this weekend and will post something by EOD (I'm UTC-5).

Hi @mpdonadio
I'm GMT+10 and was pointed at this issue by webchick and pwolanin when we did a NJ baton pass. Angie and I asked if you were still working on it in #drupal-nj and were told you were asleep. Didn't realize you had a partial patch, sorry. Feel free to ignore my changes, Lee

No worries, I should have updated the proposed resolution to show how we were going to use #2418139: Add a toUriString method to Url class and add a route: scheme to hopefully make this cleaner.

#32 Looked like a better starting point than my WIP (esp w/ the necessary changes to the unit test), so I hand merged a few changes in. This is mainly to see what testbot says, and to hand it off.

If this somehow comes up green, the next step would be to see if we can optimize the Url -> string -> Url -> string, which may only be possible for routed Urls. Perhaps, in this case we can do the token replacement on the route parameters and options arrays, and then bypass one stringization step.

poking again

the user-path no longer needed because toUriString() includes that

to do:
* path processor for language handling (or we add a kernel test for those)
* some fragment and query elements being lost, not sure if those are in views or in toUrlString()

Aah, #44-2 closes the @defgroup at the top (missed that), so it should probably read

/**
 * @} End of "defgroup views_field_handlers".
 */

I'll see if I can make any forward progress on this today during breaks. I will unassign myself and post my WIP.

Tagging out on this for the day. #44 is addressed. Didn't have time to look at the fails, so this isn't worth a TestBot run.

kicking along

this should leave just the phpunit fails

Tag, I'm it. Will post my work by end of day.

Expecting one fail in FieldPluginBaseTest::testRenderAsLinkWithPathAndOptions() with this.

I am not sure why, but the outbound path processor for prepending the language isn't running for the unrouted URL assembler. It does work for routed ones.

I am not sure if this is a mocking problem, a container problem (eg, a missing service), or something else. Oddly, I can't find a unit test that does language prefixing for an example. I suspect that PathProcessorLanguage needs to be wired up to the unrouted URL assembler, but I kept having problems with this and ran out of time today...

I edited the wrong file in #48 and this also removes a test annotation.

+1 from me, we test a lot of this in an integration test in Drupal\views\Tests\Handler\FieldWebTest, if we need language, we can add it there

+++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
@@ -1314,7 +1314,7 @@ protected function renderAsLink($alter, $text, $tokens) {
-      $alter['url'] = CoreUrl::fromUri('user-path:' . $path);
+      $alter['url'] = CoreUrl::fromUri('user-path:/' . $path);

Could be the fails are related to this?

Added
$alter['url'] = CoreUrl::fromUri('user-path:/' . ltrim($path, '/'))->toString();
Assuming this can be an blocker on previous patches

Seems some size issue in my patch will add other things as well.

+1 for your submission. Can we remove no of white lines and optimize some code while assigning values. like

$this->options['alter']['make_link'] = TRUE;
-      $this->options['alter']['path'] = $link;
+      // Note: $link is an external URI, pointing to the feed itself.
+      $this->options['alter']['url'] = Url::fromUri($link);
       $this->options['alter']['html'] = TRUE;
       $this->options['alter']['absolute'] = TRUE;

can be

$this->options['alter'] = array('make_link' => TRUE,
'path' => $link,
'url', => Url::fromUri($link),
       'html' => TRUE;
       'absolute' => TRUE);

I am not sure if it is required here.

GREEN! Awesome.

This needs a few pairs of eyes looking at the tests to double check that they are doing what we think they are.

FieldPluginBaseTest::providerTestRenderAsLinkWithUrlAndOptions() could potentially use some cleanup, but that data provider was rather tricky to get right.

@RavindraSingh, thanks for trying to help move this forward. Also, make sure you post an interdiff if your work so everyone can see what has changed. Personally, I do the git-branch-per-patch method, as I tend to do small commits on complicated issues, so this helps in other ways for me, too.

@RavindraSingh regarding the optimization question in #73,
we cannot do that in this issue. We only want to change the lines we need to do this issue,
and there are lines building that array that do not need to change.

about the white lines, I looked through the patch and they all look appropriate.
Some are required by our standards, and some are added for readability.
As long as there are not multiple white lines, we are probably ok.
https://www.drupal.org/coding-standards#indenting
https://www.drupal.org/node/608152#indenting

That sounds good for me. I have reviewed the patch seems fine to me.
great job by everyone this long long thread. Moving it to RTBC .

1. +++ b/core/modules/comment/src/Plugin/views/field/LinkDelete.php
   @@ -43,7 +44,7 @@ protected function renderLink($data, ResultRow $values) {
   +    $this->options['alter']['url'] = $comment->urlInfo('delete_form');
   

   Shouldn't this be delete-form?

2. +++ b/core/modules/user/src/Plugin/views/field/LinkCancel.php
   @@ -28,7 +28,7 @@ protected function renderLink(EntityInterface $entity, ResultRow $values) {
   +      $this->options['alter']['url'] = $entity->url('cancel_form');
   

   cancel-form

3. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
   @@ -1295,34 +1295,46 @@ public function renderTrimText($alter, $value) {
   +    // $path will be run through check_url() by the link generator so we do not
   +    // need to sanitize it ourselves.
        $path = $alter['path'];
   +    if (empty($alter['url'])) {
   +      if (!parse_url($path, PHP_URL_SCHEME)) {
   +        $alter['url'] = CoreUrl::fromUri('user-path:/' . ltrim($path, '/'));
   +      }
   +      else {
   +        $alter['url'] = CoreUrl::fromUri($path);
   +      }
   +    }
   

   The comment feels misleading/wrong. It feels like we should say that we convert $alter['path'] to a user-path URI. And perhaps that the Url class takes care of access checking. (Not sanitization.)

4. +++ b/core/modules/views/tests/src/Unit/Plugin/field/FieldPluginBaseTest.php
   @@ -0,0 +1,445 @@
   +  protected function setUp() {
   

   Missing {@inheritdoc}.

5. +++ b/core/modules/views/tests/src/Unit/Plugin/field/FieldPluginBaseTest.php
   @@ -0,0 +1,445 @@
   +    // Mock a url generator to make it working properly.
   

   This comment can be deleted; it's poorly written and doesn't add anything.

6. +++ b/core/modules/views/tests/src/Unit/Plugin/field/FieldPluginBaseTest.php
   @@ -0,0 +1,445 @@
   +   * Set ups the unrouted url assembler and the link generator.
   

   s/Set ups/Sets up/
   s/url/URL/

7. +++ b/core/modules/views/tests/src/Unit/Plugin/field/FieldPluginBaseTest.php
   @@ -0,0 +1,445 @@
   +  protected function setupUrlIntegrationServices() {
   

   s/setup/setUp/

I'll take care of #77 later today (I wanted to look at this in PhpStorm anyway), but further reviews appreciated. Also very curious why #1 and #2 weren't caught by tests? #2 is really wrong since is calls url() and not urlInfo().

I am nearly sure the comment on #3 is leftover from the old code where _url() was called and this was a proper comment.

Crosspost. Will let @dawehner take care of this.

This just fixes another link annotation URL. I did a search and eyeballed the others, and they look OK.

I'm just confused as I thought I remembered seeing tests for these when I was working on the big _url() issue, but it looks like the comment list is still code and not a view, and the user list view doesn't have that link directly on it in each row. I get to them when I can, but it may be a few days.

1. +++ b/core/modules/aggregator/src/Plugin/views/field/TitleLink.php
   @@ -78,7 +79,8 @@ protected function renderLink($data, ResultRow $values) {
   +      $this->options['alter']['url'] = Url::fromUri($link);
   
   +++ b/core/modules/comment/src/Plugin/views/field/LinkApprove.php
   @@ -51,8 +52,8 @@ protected function renderLink($data, ResultRow $values) {
   -    $this->options['alter']['path'] = "comment/" . $comment->id() . "/approve";
   -    $this->options['alter']['query'] = drupal_get_destination() + array('token' => \Drupal::csrfToken()->get($this->options['alter']['path']));
   +    $this->options['alter']['url'] = $comment->urlInfo('approve');
   +    $this->options['alter']['query'] = drupal_get_destination() + array('token' => \Drupal::csrfToken()->get($this->options['alter']['url']->toString()));
   

   Not sure how this works. I can't find an approve link template.

2. +++ b/core/modules/node/src/Plugin/views/field/RevisionLinkDelete.php
   @@ -50,7 +51,7 @@ protected function renderLink($data, ResultRow $values) {
   +    $this->options['alter']['url'] = Url::fromRoute('node.revision_delete_confirm', ['node' => $node->id(), 'node_revision' => $vid]);
   
   +++ b/core/modules/views/src/Plugin/views/PluginBase.php
   @@ -346,6 +346,10 @@ protected function viewsTokenReplace($text, $tokens) {
   +    // Unescape Twig delimiters that may have already been escaped.
   +    // @see \Drupal\viewsViewExecutable::getUrl().
   +    $text = str_replace(['%7B','%7D'], ['{','}'], $text);
   

   huh?

I'll fix #88-1, and double check the need for #2 and explain it more better in the comment (I wrote that hunk). I'll probably do both later today.

Note that if #2417567: Rename user-path: scheme to internal: lands before this, the patch will need to be rerolled to take into account the explicit Url::fromUri('user-path') calls. However, I have Alex's feedback addressed w/ a green patch and will post tonight (the second point requires a bit of explanation that I don't have time for right now).

TL;DR, #88 is addressed with this patch.

#1, yeah no annotation; converted to fromRoute().

#2 need explanation. FieldPluginBase::renderAsLink() calls viewsTokenReplace() a whole bunch of times for token replacement. There is a system view in views.view.files.yml, view.files.page_2, that has an path defined with an segment that is actually a Twig token. This quirk is what led to the ::toUriString() issue...

Url::toUriString() uses UrlHelper::buildQuery() internally to build up the argument list for routes. That, uses rawurlencode(). The net result is that

\Drupal\Core\Url::fromUri('route:view.files.page_2;arg_0={{fid}}')->toUriString() == 'route:view.files.page_2;arg_0=%7B%7Bfid%7D%7D';

So, we needed to decode those escaped values in order to token replacement. Instead of doing a total decode, we just string replaced those two cases. At the time, we thought doing this in viewsTokenReplace() for all tokens was best, but moving this just to the token replacement for paths probably is best (which is what this patch does).

\Drupal\file\Tests\FileListingTest is where we noticed this problem.

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 7b5e5a9 and pushed to 8.0.x. Thanks!

diff --git a/core/modules/views/src/Plugin/views/field/FieldPluginBase.php b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
index d683ea7..a0e1211 100644
--- a/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
+++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
@@ -1328,10 +1328,9 @@ protected function renderAsLink($alter, $text, $tokens) {
     if ($path != 'route:<front>') {
       // Unescape Twig delimiters that may have been escaped by the
       // Url::toUriString() call above, because we support twig tokens in
-      // rewrite settings of views fields.
-      // In that case the original path looks like
-      // user-path:/admin/content/files/usage/{{fid}}, which will be escaped by
-      // the toUriString() call above.
+      // rewrite settings of views fields. In that case the original path looks
+      // like user-path:/admin/content/files/usage/{{fid}}, which will be
+      // escaped by the toUriString() call above.
       $path = str_replace(['%7B','%7D'], ['{','}'], $path);
 
       // Use strip tags as there should never be HTML in the path.

I re-flowed the comment on commit.

This introduced a small regression for twig placeholders, please review #2426447: Views no longer supports {{ something }} as twig placeholder for a path, only {{something}}

Note: this issue / patch seems to have broken all the transformation options in renderAsLink() when the path you're trying to link to happens to be a route. See #2645954-26: Views output field as a custom link options all ignored if path is routed for more. Anyone who knows more about WTF is going on with this code is hereby invited to comment on / shred my latest patch there. But it does work. ;)

Thanks,
-Derek