Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unsupported semicolon prefix in LinkGenerator::link calls #255

Open
dakujem opened this issue Mar 24, 2020 · 11 comments · May be fixed by #256
Open

Unsupported semicolon prefix in LinkGenerator::link calls #255

dakujem opened this issue Mar 24, 2020 · 11 comments · May be fixed by #256

Comments

@dakujem
Copy link

dakujem commented Mar 24, 2020

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]"
@mabar
Copy link
Contributor

mabar commented Mar 24, 2020

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

@dakujem
Copy link
Author

dakujem commented Mar 24, 2020

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.

@dakujem dakujem changed the title Inconsistent reaction to deprecated link's destination format Unsupported semicolon prefix in LinkGenerator::link prefix Mar 24, 2020
@dakujem dakujem changed the title Unsupported semicolon prefix in LinkGenerator::link prefix Unsupported semicolon prefix in LinkGenerator::link calls Mar 24, 2020
@dakujem dakujem linked a pull request Mar 24, 2020 that will close this issue
@dakujem
Copy link
Author

dakujem commented Mar 24, 2020

You're welcome.

@dg
Copy link
Member

dg commented Mar 26, 2020

Duplicated to #72 and #92

@dakujem
Copy link
Author

dakujem commented Mar 26, 2020

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.

@mabar
Copy link
Contributor

mabar commented Mar 26, 2020

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

@dakujem
Copy link
Author

dakujem commented Mar 26, 2020

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));

@mabar
Copy link
Contributor

mabar commented Sep 25, 2021

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...

@MartinMystikJonas
Copy link

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.

@dakur
Copy link
Contributor

dakur commented Nov 8, 2023

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

@dakujem
Copy link
Author

dakujem commented Nov 8, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants