Skip to content

[Routing] Fixed the interface description of the url generator interface #28321

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

Merged
merged 1 commit into from
Sep 4, 2018

Conversation

Toflar
Copy link
Contributor

@Toflar Toflar commented Aug 31, 2018

Q A
Branch? 2.8
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

The UrlGenerator has always been able to return null. Many tests assert this for many years but the interface actually always only allowed a string return.

Examples for tests:

So I think I would not consider this change as a BC break but rather a doc fix because it seems like null has always been an accepted return value.

@nicolas-grekas nicolas-grekas changed the title Fixed the interface description of the url generator interface [Routing] Fixed the interface description of the url generator interface Aug 31, 2018
@linaori
Copy link
Contributor

linaori commented Aug 31, 2018

Can we throw a deprecation when it returns null and adjust the signature to : string in 5.0? It doesn't make sense to return null here, you'd expect an exception in the URL generation.

@ro0NL
Copy link
Contributor

ro0NL commented Sep 1, 2018

if ($this->strictRequirements) {
throw new InvalidParameterException($message);
}
if ($this->logger) {
$this->logger->error($message);
}
return;

This is the only case, and AFAIK intended to not throw. Currently null allows you to differ this case from a valid "" URL. However im not sure we should care (it's logged) and perhaps we can always return string then... deprecting null first 👼

At least in practice i think this null is already casted to string in app code.

* @return string
*/
public function getPath($name, $parameters = array(), $relative = false)
{
return $this->generator->generate($name, $parameters, $relative ? UrlGeneratorInterface::RELATIVE_PATH : UrlGeneratorInterface::ABSOLUTE_PATH);
}
/**
* @param string $name
* @param array $parameters
* @param bool $schemeRelative
*
* @return string
*/
public function getUrl($name, $parameters = array(), $schemeRelative = false)
{
return $this->generator->generate($name, $parameters, $schemeRelative ? UrlGeneratorInterface::NETWORK_PATH : UrlGeneratorInterface::ABSOLUTE_URL);

@Tobion
Copy link
Contributor

Tobion commented Sep 2, 2018

I agree returning null does not make sense for this method. It could just be changed to an empty string when not strictRequirements

@fabpot
Copy link
Member

fabpot commented Sep 3, 2018

I agree with @Tobion

@aschempp
Copy link
Contributor

aschempp commented Sep 3, 2018

Would this be a bugfix then (target against 2.8 branch)?

@fabpot
Copy link
Member

fabpot commented Sep 4, 2018

Merging as is in 2.8, changing null should be done in master.

@fabpot
Copy link
Member

fabpot commented Sep 4, 2018

Thank you @Toflar.

@fabpot fabpot merged commit d2e9e0b into symfony:2.8 Sep 4, 2018
fabpot added a commit that referenced this pull request Sep 4, 2018
…erator interface (Toflar)

This PR was merged into the 2.8 branch.

Discussion
----------

[Routing] Fixed the interface description of the url generator interface

| Q             | A
| ------------- | ---
| Branch?       | 2.8
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

The `UrlGenerator` has always been able to return `null`. Many tests assert this for many years but the interface actually always only allowed a `string` return.

Examples for tests:

- https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Routing/Tests/Generator/UrlGeneratorTest.php#L206
- https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Routing/Tests/Generator/UrlGeneratorTest.php#L217
- https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Routing/Tests/Generator/UrlGeneratorTest.php#L471

So I think I would not consider this change as a BC break but rather a doc fix because it seems like `null` has always been an accepted return value.

Commits
-------

d2e9e0b Fixed the interface description of the url generator interface
@Tobion
Copy link
Contributor

Tobion commented Sep 4, 2018

This should not have been merged IMO. Now people will say it's a bc break to change the interface in master again when null wasn't even documented for years.

@fabpot
Copy link
Member

fabpot commented Sep 5, 2018

Right, reverted.

fabpot added a commit that referenced this pull request Sep 5, 2018
… url generator interface (Toflar)"

This reverts commit 487f8ac, reversing
changes made to cf359c2.
nicolas-grekas added a commit that referenced this pull request Sep 5, 2018
* 2.8:
  [appveyor] fix
  Revert "minor #28321 [Routing] Fixed the interface description of the url generator interface (Toflar)"
  remove cache warmers when Twig cache is disabled
  [HttpKernel][FrameworkBundle] Fix escaping of serialized payloads passed to test clients
  chore: rename Appveyor filename
  Fixed the interface description of the url generator interface
  Format file size in validation message according to binaryFormat option
nicolas-grekas added a commit that referenced this pull request Sep 5, 2018
* 3.4:
  [appveyor] fix
  Revert "minor #28321 [Routing] Fixed the interface description of the url generator interface (Toflar)"
  Fixed caching of templates in default path on cache warmup
  remove cache warmers when Twig cache is disabled
  [HttpKernel][FrameworkBundle] Fix escaping of serialized payloads passed to test clients
  chore: rename Appveyor filename
  Fixed the interface description of the url generator interface
  Format file size in validation message according to binaryFormat option
nicolas-grekas added a commit that referenced this pull request Sep 5, 2018
* 4.1:
  [appveyor] fix
  [DI] Fix dumping some complex service graphs
  Revert "minor #28321 [Routing] Fixed the interface description of the url generator interface (Toflar)"
  Fixed caching of templates in default path on cache warmup
  added missing LICENSE file
  remove cache warmers when Twig cache is disabled
  [Workflow] Make sure we do not run the next transition on an updated state
  change baseUrl to basePath to fix wrong profiler url
  [HttpKernel][FrameworkBundle] Fix escaping of serialized payloads passed to test clients
  chore: rename Appveyor filename
  Fixed the interface description of the url generator interface
  Format file size in validation message according to binaryFormat option
@Toflar Toflar deleted the doc-fix branch September 24, 2018 06:47
leofeyer pushed a commit to contao/contao that referenced this pull request Oct 8, 2018
Description
-----------

As outlined in symfony/symfony#28321 we cannot rely on the url generator of the router to always return a string. This was an issue in the interface docs of Symfony.

Commits
-------

5aaf547 Fixed unhandled null case in contao framework
ef39a73 Added unit test
10268b1 Rename the test method
leofeyer pushed a commit to contao/core-bundle that referenced this pull request Oct 8, 2018
Description
-----------

As outlined in symfony/symfony#28321 we cannot rely on the url generator of the router to always return a string. This was an issue in the interface docs of Symfony.

Commits
-------

5aaf5471 Fixed unhandled null case in contao framework
ef39a735 Added unit test
10268b10 Rename the test method
@xabbuh
Copy link
Member

xabbuh commented Mar 8, 2019

I agree returning null does not make sense for this method. It could just be changed to an empty string when not strictRequirements

FYI, this was implemented in #30390

fabpot added a commit that referenced this pull request Nov 9, 2019
…ce::generate to remove null (shieldo)

This PR was merged into the 3.4 branch.

Discussion
----------

[Routing] revert the return type for UrlGeneratorInterface::generate to remove null

…to remove null

| Q             | A
| ------------- | ---
| Branch?       | 3.4 (only)
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| License       | MIT

Bit of a casualty of commit tennis this:

A change to add `null` here as an option for how `UrlGeneratorInterface::generate()` (rather than the concrete `UrlGenerator`) was merged in #28321, but then [reverted](90494c2) for the reason [that this could be seen as a BC break](#28321 (comment)), as the `null` return had not previously been documented (and is still not as part of the interface method docs).

However, in a subsequent change (#33252) with a wider scope, this doc change was added _back_ in order to reflect the underlying implementation as a result of a PHPStorm plugin complaining. There's no indication though of what a `null` return here though would mean, and for the same reason as the first revert (that this should be seen as a BC break), I'd like to submit this to be reverted for the 3.4 branch. (In 4.4 the `null` has already been removed.)

Having the interface indicating that this method can return `null` necessitates introducing a lot of actually redundant null checks in code that is covered by static analysis tools such as PHPStan.

Commits
-------

9f853f3 [Routing] revert the return type for UrlGeneratorInterface::generate to remove null
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants