application

application copied to clipboard

Version: 3.0.4

Links with destination in deprecated/invalid format
:Module:Presenter:action (note the semicolon : at the beginning of the destination string)
work with UI\Presenter/UI\Component but not with LinkGenerator.

I'm not sure which one is the desired behaviour, or whether this is a bug of UI\Component or LinkGenerator class, but in the following scenario, only the first call works:

$presenter->link(':Foo:Bar:'); // works ✔
$linkGnrtr->link(':Foo:Bar:'); // fails ❌

The second call fails with Presenter name must be alphanumeric string, ':Foo:Bar' is invalid..

Links in the correct format work with both classes, of course ✔:

$presenter->link('Foo:Bar:'); // works ✔
$linkGnrtr->link('Foo:Bar:'); // works ✔

I understand that the format is not correct according to current documentation, but I would like to point out that the behaviour should be consistent, that is,
it should either work in both cases or not work in either one.

EDIT:
I forgot to add that calling

$presenter->getAction(true);

will result in (depending on whether the presenter is in a module or not)

':Foo:Bar:default' // inside a module
':Bar:default'     // outside a module

... which is in direct contradiction to the format, as in the methods' comments:

@param  string   $dest in format "[[[module:]presenter:]action] [#fragment]"

Leading : in presenter/component means that absolute name should be used instead of relative name. LinkGenerator always uses absolute one so : would make no difference.

LinkGenerator may simply trim leading :, so exact same absolute link could be used anywhere

Thank you @mabar I have just discovered that I actually need the leading : because I need to be able to create inter-module links. I will change the tilte of this issue accordingly.

Still, are you suggesting that I should trim the : prefix if I want to use LinkGnerator class instead of trimming it in the class itself? I believe since the :-prefixed links do work with presenters/components, they should also work with the generator class.

You're welcome.

Duplicated to #72 and #92

Okay, so we have several people who have encountered this inconsistency spread across several versions (several years between the mentioned issues) and now we have a PR that deals with the issue, is test covered and fixes the issue with performance cost of one if ($presenter[0] === ':') statement without any BC impact ... and it's going to be scrapped for ... what reason in fact?

Sidenote: May I point out that both UI\Presenter::createRequest and UI\Component::link methods have incorrect dumentation of the $destination parameter:

@param  string   $destination in format "[//] [[[module:]presenter:]action | signal! | this] [#fragment]"

It's missing the leading semicolon option.

My personal opinion is, if there should be only one syntax allowed, then LinkGenerator should accept only links with leading colon and deprecate syntax without it. Allowing both syntaxes would lead to inconsistency in LinkGenerator, but currently inconsistency is in usage of absolute links in Component and in LinkGenerator. It was always confusing for me that relative links in Component and absolute links in LinkGenerator looks the same and leaded me to several errors and it also complicates usage of same links in different contexts

For me, the deal breaker is this call, that fails. It's one of those WTF moments I would like to avoid.

(new LinkGenerator(...))->link($presenter->getAction(true));

~~It's possible to just override PresenterFactory, because $name is passed by reference.~~

public function getPresenterClass(string &$name): string
{
	if (str_starts_with($name, ':')) {
		$name = substr($name, 1);
	}

	return parent::getPresenterClass($name);
}

~~(probably) related feature #296~~

Edit: It of course works only in combination with presenter factory...

I think LinkGenerator shoukd accept both formats. It is unnecessarily annoying to use two different formats for absolute paths or convert them. Especially when same value needs to be used in both presenter and link generator.

I prepare links in different place than I later call link() (see below), but I can't do it service-agnostic because LinkGenerator doesn't support leading :

@dg Could you please reconsider the closing of #92 or at least share your thoughts on why you have declined it? Thank you.

My usage:

// SomePresenter:
public static function linkParams(Id $id, Version $version): array
{
    return [':Some:default', [
        'id' => $id,
        'version' => $version,
    ]];
}

// AnotherPresenter:
$this->link(...SomePresenter::linkParams($id, $version)); // ok
// Component or other non-presenter context:
$this->linkGenerator->link(...SomePresenter::linkParams($id, $version)); // error

So annoying to come to this notification to see a 3.5 years old issue still unresolved.