Fix the typehinting in CurrentPathStack

Please find the attached patch.
Thanks.

@jibran Thanks for mentoring and for the review.

So adding a typehint can break BC if a passed-in value does not match the typehint. This class is basically a value object, so as far as I can tell it doesn't actually fall under any of the categories defined in https://www.drupal.org/core/d8-bc-policy as internal. (There is no corresponding interface; it's not explicitly marked as internal; the methods are public and not the constructor, etc.) The only thing that might address it is the generalized note in https://www.drupal.org/core/d8-bc-policy#classes but I think that was intended to be about about classes with a corresponding interface, not value objects. I'm not actually sure it's even really internal at all, as it's used in places like Views (okay, admittedly a bit special), a condition plugin, etc. in addition to the router.

Since it falls back to the current request if not passed, code that didn't expect it to be such an object would presumably be pretty strange and broken. Obviously the parameter is intended to be a request object. But maybe we should at least add a small change record since it's not actually internal API?

Changing tags

Created https://www.drupal.org/node/2887199 for #5.

Please add @jibran to the commit mentioned for helping me writing the change record and creating the patch.

I took a look into whether this is a BC break, but in actual fact - if you pass anything other than a Request object, it will result in a fatal error.

Here's the code of the getPath() method

public function getPath($request = NULL) {
    if (!isset($request)) {
      $request = $this->requestStack->getCurrentRequest();
    }
    if (!isset($this->paths[$request])) {
      $this->paths[$request] = $request->getPathInfo();
    }

    return $this->paths[$request];
  }

Note
$this->paths[$request] = $request->getPathInfo();

If you pass anything that doesn't have a ::getPathInfo method, then you're going to cause a fatal.

So looking into other objects in core that have a getPathInfo method

\Symfony\Component\HttpKernel\DataCollector\RequestDataCollector::getPathInfo
\Symfony\Component\Routing\RequestContext::getPathInfo
\SplFileInfo::getPathInfo

Whilst in theory, you could pass any of those - its not intended that you would

In terms of the setPathInfo method, note that $this->paths on the CurrentPathStack is an object of type \SplObjectStorage so in other words, effectively an array keyed by objects.

Therefore in order for that to work, what you call setPath with, has to match what you call getPath so because we are typehinting getPath to prevent the fatal, it follows that we should typehint setPath too, in order for the \SplObjectStorage object to work as intended.

Because of that, I believe that in this instance the BC break is warranted.

FWIW: This code was added in #2372507: Remove _system_path from $request->attributes which was a critical

I agree with #9; the typehint should just make the existing implicit requirement more explicit. But since it is public API, just in case someone has managed to use it in an unexpected way for some unimagined usecase, the change record and making it a minor-only change should give sufficient indication.

I improved the CR a bit to clarify why we were making the change and that it's unlikely to impact code that already uses the API:
https://www.drupal.org/node/2887199/revisions/view/10528629/10529311

Thanks @pasan.gamage for the patch and @jibran for the mentoring! Saving this comment before committing because the crediting widget isn't updating properly.

Committed to 8.4.x. Congratulations @pasan.gamage on your first core contribution!

Thanks @xjm :)

@jibran: Thanks for getting the change record; I should have remembered to publish it. :)

I do agree that it's a bug. As I mentioned above, though, there is a very tiny chance of disruption for some weird and unexpected implementation (since it's technically a BC break for a public API and someone could have a weird implementation), so we do not backport such fixes to patch releases. Thanks!