unmaskUrl method: private to protected

[nietzscheson]

This would provide the ability to modify how routes and parameters are generated. It could easily extend and inject the services that are required to create the route depending on the project. Example:

abstract class SymfonyPage extends Page
{

  private $router;

  public function __construct(Session $session, Factory $factory, array $parameters = array(), RouterInterface $router)
  {
    parent::__construct($session, $factory, $parameters);

    $this->router = $router;
  }

  protected function unmaskUrl(array $urlParameters)
  {
    return $this->router->generate($this->getPath(), $urlParameters);
  }
}

[DonCallisto]

Why a canonical open($url, $params) is not suitable here?
Just because you're defining the $path as a symfony route and not as a url, am I right?

If so, I suppose you should provide an extension or something like this: this bundle isn't base on symfony so, the only advantage of this visibility change, seems to suite symfony user needs and I vote to keep this outside this extension and put it elsewhere.

Moreover you can easily override open function to obtain what you need.

Don't know if @jakzal agree with me or not

[nietzscheson]

Okay, it can lend itself to confusion. But, I think it would benefit those who use route components within our projects, such as Symfony or Laravel or any other

With this change, the package will not be bound to any framework. If someone does not use a router packet or needs to generate the routes, you can still use it as-is.

This opens the possibility of creating a SymfonyRouteExtension, based on page objects, for Behat .

[nietzscheson]

An alternative not to make this change, when using a componenten router is:

abstract class SymfonyPage extends Page
{

  private $router;

  public function __construct(Session $session, Factory $factory, array $parameters = array(), RouterInterface $router)
  {
    parent::__construct($session, $factory, $parameters);

    $this->router = $router;
  }

  protected function getPath()
  {
     if (null === $this->path) {
	throw new PathNotProvidedException('You must add a path property to your page object');
     }

     return $this->router->getRouteCollection()->get($this->path)->getPath();
  }
}

[DonCallisto]

But, I think it would benefit those who use route components within our projects, such as Symfony or Laravel or any other

In my humblest opinion and for this reason this should be an extension.

With this change, the package will not be bound to any framework. If someone does not use a router packet or needs to generate the routes, you can still use it as-is.

I'm pretty sure that overriding open could be a good way to achieve this

[nietzscheson]

I would not change the open. This is where the route parameters are entered. Context should know nothing of the route; yes the parameters: these could be dynamic.

If you do not get the change to unmaskUrl()method, the alternative is that who needs to modify the way the routes are created, is to modify getPath(); everything works well...

The SymfonyPage class will not be embedded within PageObjects: it is a solution to which we use routing components. To be able to extend our pages:

<?php

namespace Behat\Page\Security;

use Behat\Page\SymfonyPage;

class LoginPage extends SymfonyPage implements LoginPageInterface
{
   protected $path = 'fos_user_security_login';
}

[DonCallisto]

Context should know nothing of the route

If you need to open a page with a certain parameter, even with your method or the way it's already defined, Context should be aware of the parameters. There's no other way unless parameters are hardcoded in the page.

[jakzal]

Initially I thought this is a good idea, but after reading @DonCallisto's comments and looking at the code again I think we already have proper extension points in place - getUrl() and possibly getPath() (I don't remember why getPath() was made protected now.

Currently overriding getUrl() is most suitable for what you need.

Could we improve this though?

getPath(), makeSurePathIsAbsolute() and unmaskUrl() are implementation details of the default route generation strategy. Looking at it now the Page class has an additional responsibility of generating routes. It violates SRP (which was fine until now).

Instead, you could imagine having a page rely on some kind of a Router or UrlGenerator and getUrl() proxying to that router. The three private methods would be defined on the UrlGenerator, and your implementation wouldn't use them as it would provide it's own way of generating paths.

abstract class Page extends DocumentElement implements PageObject
{
    public function __construct(Session $session, Factory $factory, array $parameters = array(), UrlGenerator $urlGenerator)
    {
        parent::__construct($session);
        $this->factory = $factory;
        $this->parameters = $parameters;
        $this->urlGenerator = $urlGenerator;
    }

   // ...

    protected function getUrl(array $urlParameters = array())
    {
        return $this->urlGenerator->generateUrl($this->getPath(), $urlParameters);
    }
}

interface UrlGenerator
{
   // not sure how to call the first argument - $locator, $name, $path?
    public function generateUrl($locator, array $urlParameters = array()): string;
}

class DefaultUrlGenerator implements UrlGenerator
{
    public function generateUrl($locator, array $urlParameters = array()): string
    {
         return $this->makeSurePathIsAbsolute($this->unmaskUrl($name, $urlParameters));
    }

    private function makeSurePathIsAbsolute($path)
    {
       // this is actually not here, perhaps it should be passed via the constructor
        $baseUrl = rtrim($this->getParameter('base_url'), '/').'/';

        return 0 !== strpos($path, 'http') ? $baseUrl.ltrim($path, '/') : $path;
    }
    
    private function unmaskUrl($url, array $urlParameters)
    {
        foreach ($urlParameters as $parameter => $value) {
            $url = str_replace(sprintf('{%s}', $parameter), $value, $url);
        }

        return $url;
    }
}

WDYT?

[nietzscheson]

I like it a lot ... it will be cleaner. Less confusing. More decoupled. It offers the possibility to extend the package to create new extensions from this...

[nietzscheson]

Now, the implementation of $path must be abstract as it says @DonCallisto, and must be defined in this generator. Thus, the extensions can be the generic name they want as:

• abstract public function getRouteName();
• abstract public function getPath();

[jakzal]

getPath can't be abstract as I don't want to force people to use it. Not every page will be open()ed. Some are just returned from other pages.

[DonCallisto]

@jakzal so, basically, you can define your own UrlGenerator that implements UrlGenerator and use it. Totally decoupled and opened to extension. If I'm right here, I like it a lot and I agree with you.

However this getPath is something difficult to handle: path has sense only within a strategy (here default one) and, moreover, forces the name of the variable.
I'm aware that not all pages are opened directly and if is true that in this case it's not useful and noisy, in other cases will become more fragile to write code the way it is now and the way it could be with this enhancement.

Only solution I found here is to create an interface and two different types of pages: one that won't be open, won't be that function, others will. Don't know if is possible and if is straightforward and no over-engeneerized.

WDYT?

[jakzal]

@jakzal so, basically, you can define your own UrlGenerator that implements UrlGenerator and use it. Totally decoupled and opened to extension.

Exactly.

However this getPath is something difficult to handle: path has sense only within a strategy (here default one) and, moreover, forces the name of the variable.
I'm aware that not all pages are opened directly and if is true that in this case it's not useful and noisy, in other cases will become more fragile to write code the way it is now and the way it could be with this enhancement.

We need to keep BC here. It's fine if we improve the situation a little bit for now and keep this in mind for future iterations. The path property doesn't have to be used at all by people who provide the UrlGenerator implementation. Short term it's fine by me.

Only solution I found here is to create an interface and two different types of pages: one that won't be open, won't be that function, others will. Don't know if is possible and if is straightforward and no over-engeneerized.

Looking at how Mink code is being refactored we might need to stop extending their classes. It's hardly ever good idea to do it, yet I've done it here as it was very convinient for the initial audience of the extension - testers (this is also part of the reason why $path is a property instead of getPath() being abstract). This is something we'll need to consider for a Behat-decoupled implementation (which this extension will also start using).