Views methods FieldPluginBase::renderAsLink and renderText() uses base_path() when it should use $context->getBaseUrl()

+1

I think all of those fails are from the unit tests. I guess the question is whether to just add the mock so the tests pass using a base path of /, or use a mock with a non-/ basepath and update tests to explicitly cover this. The later is probably better.

First pass at adding missing dependency in container.

#9, nice patch (even though it was left at NW, the test passed). Totally forgot this test uses the ContainerBuilder.

There is another use of base_path() in that class related to links that should really go, and not increase the scope of the issue. It also allows us to get rid of the hack at the end of the test.

So the question remains, do we want to keep this as-is, or adjust the test to explicitly check for non-/ base paths?

Looked like fails were b/c #2790685: FieldHandlersUpdateTest has random test fails, https://www.drupal.org/pift-ci-job/506881

+++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
@@ -102,6 +104,40 @@
+   * @var \Symfony\Component\Routing\RequestContext
+   */
+  protected $context;
+

+++ b/core/modules/views/tests/src/Unit/Plugin/field/FieldPluginBaseTest.php
@@ -114,6 +116,13 @@ class FieldPluginBaseTest extends UnitTestCase {
+   * @var \Drupal\Core\Routing\RequestContext
+   */
+  protected $requestContext;
+

Think I made that last change. w/o looking at the rest of the patch. For consistency sake, which is preferred `$context` or `$requestContext`? There doesn't seem to be a clear winner in core, but it looks like the later may be used more (which is why I made the change, but missed the others in the patch).

1. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
   @@ -102,6 +104,40 @@
   +    $this->context = $context;
   
   @@ -1824,6 +1862,18 @@ protected function getRenderer() {
   +   * Returns the request context.
   +   *
   +   * @return \Symfony\Component\Routing\RequestContext
   +   */
   +  protected function getRequestContext() {
   +    if (!isset($this->context)) {
   +      $this->context = \Drupal::service('router.request_context');
   +    }
   +
   +    return $this->context;
   +  }
   

   An alternative pattern is to lazy fetch it in the constructor, if it wasn't passed long. Not sure what is better to be honest.

2. +++ b/core/modules/views/tests/src/Unit/Plugin/field/FieldPluginBaseTest.php
   @@ -726,12 +797,3 @@ public function setLinkGenerator(LinkGeneratorInterface $link_generator) {
    
    }
   -
   -// @todo Remove as part of https://www.drupal.org/node/2529170.
   -namespace Drupal\views\Plugin\views\field;
   -
   -if (!function_exists('base_path')) {
   -  function base_path() {
   -    return '/';
   -  }
   -}
    
   

   It is super nice to get rid of this hack.

+++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
@@ -102,6 +104,40 @@
+  public function __construct(array $configuration, $plugin_id, $plugin_definition, RequestContext $context = NULL) {
+    parent::__construct($configuration, $plugin_id, $plugin_definition);
+    $this->context = $context;

We need to add bc warning and bc test for this change.

+++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
@@ -1824,6 +1862,18 @@ protected function getRenderer() {
+  protected function getRequestContext() {
+    if (!isset($this->context)) {
+      $this->context = \Drupal::service('router.request_context');
+    }

We usually set this in the constructor for new dependencies now, so let's follow that pattern here too and we can probably remove this method.

I did the reroll of the patch and I addressed comment #31 too.

Fixed the custom command error in #34, thanks!

Sorry, messed up the interdiff file.

I run the phpcs and in the test files and the core/modules/views/tests/src/Unit/Plugin/field/FieldPluginBaseTest.php returned this errors in the added lines:

 401 | ERROR   | [ ] Missing short description in doc comment
 405 | ERROR   | [x] Parameter comment must end with a full stop
 415 | ERROR   | [ ] The array declaration extends to column 93 (the limit is 80). The array content should be split up over multiple lines
 445 | ERROR   | [ ] The array declaration extends to column 99 (the limit is 80). The array content should be split up over multiple lines
 447 | ERROR   | [ ] The array declaration extends to column 83 (the limit is 80). The array content should be split up over multiple lines
 449 | ERROR   | [ ] The array declaration extends to column 104 (the limit is 80). The array content should be split up over multiple lines
 

also the test is returning getBaseUrl() as NULL in all the assertions related.

I will try to work on this but as I am not 100% sure how to fix it, if someone wants to step in is welcome.

Fixed the issues which is mentioned in #39. Please review.

I reviewed #40, worked on the errors and also worked on what #39 pointed out.

Kindly review it :)