[DependencyInjection] Remove inaccurate `@throws` on `Container::get()` #51835

VincentLanglet · 2023-10-04T08:14:16Z

Q	A
Branch?	6.4
Bug fix?	no
New feature?	no
Deprecations?	no
Tickets	Fix #51834
License	MIT

This would solve the inconsistency between ContainerInterface and Container described in #51834 because currently the Container phpdoc doesn't respect the ContainerInterface one.

derrabus · 2023-10-04T08:34:51Z

Wouldn't we violate PSR-11 if we upcast ServiceNotFoundException?

VincentLanglet · 2023-10-04T08:56:36Z

Wouldn't we violate PSR-11 if we upcast ServiceNotFoundException?

Sorry, I'm not sure to understand ; can you develop ?

Currently PSR\Container\ContainerInterface have the following phpdoc:

interface ContainerInterface
{
    /**
     * Finds an entry of the container by its identifier and returns it.
     *
     * @param string $id Identifier of the entry to look for.
     *
     * @throws NotFoundExceptionInterface  No entry was found for **this** identifier.
     * @throws ContainerExceptionInterface Error while retrieving the entry.
     *
     * @return mixed Entry.
     */
    public function get(string $id);
}

So, to be PSR, Symfony should only throw NotFoundExceptionInterface and ContainerExceptionInterface.

Currently, since Symfony allows to throws \Exception in the Container::get method, in my understanding the Symfony Container doesn't fully respect the PSR interface.

The introduced ServiceLoadingException implements ContainerExceptionInterface so it will now respect the PSR interface.

src/Symfony/Component/DependencyInjection/Container.php

VincentLanglet · 2023-10-09T12:36:08Z

Is this PR ok @derrabus @stof ? I'm not sure but failures seem unrelated

nicolas-grekas · 2023-10-27T08:11:33Z

src/Symfony/Component/DependencyInjection/Container.php

@@ -199,10 +201,6 @@ public function has(string $id): bool
    /**
     * Gets a service.
     *
-     * @throws ServiceCircularReferenceException When a circular reference is detected
-     * @throws ServiceNotFoundException          When the service is not defined
-     * @throws \Exception                        if an exception has been thrown when the service has been resolved


I would simply remove this line and nothing else.

This would kinda hide the problem.

Currently there is a reproducer in the Symfony tests:

symfony/src/Symfony/Component/DependencyInjection/Tests/ContainerTest.php

Lines 334 to 348 in ce48fbe

public function testGetThrowsException()

{

$this->expectException(\Exception::class);

$this->expectExceptionMessage('Something went terribly wrong!');

$c = new ProjectServiceContainer();

try {

$c->get('throw_exception');

} catch (\Exception $e) {

// Do nothing.

}

// Retry, to make sure that get*Service() will be called.

$c->get('throw_exception');

}

symfony/src/Symfony/Component/DependencyInjection/Tests/ContainerTest.php

Lines 491 to 494 in ce48fbe

protected function getThrowExceptionService()

{

throw new \Exception('Something went terribly wrong!');

}

Should it be Symfony responsibility to try/catch and rethrow a compliant exception or should we rely on the developer to throw the right exception ?

The first solution seemed safer

@throws is to document exceptions that can be thrown under the responbility of the called method. Re-thrown exceptions don't fit this definition so this @throws is just wrong. Wrapping them all won't provide anything more.

To me the purpose of the @throws tag is to know which Exceptions I should catch when doing

$container->get()

That's also how static analysis like PHPStan works.

But I'm ok removing the \Exception line, it solves my issue.
Pr is update

nicolas-grekas · 2023-10-30T13:58:44Z

Thank you @VincentLanglet.

carsonbot added Status: Needs Review Feature labels Oct 4, 2023

carsonbot added this to the 6.4 milestone Oct 4, 2023

VincentLanglet mentioned this pull request Oct 4, 2023

[DependencyInjection] ContainerInterface::get and Container::get throws tag are not consistent #51834

Closed

stof reviewed Oct 4, 2023

View reviewed changes

src/Symfony/Component/DependencyInjection/Container.php Outdated Show resolved Hide resolved

VincentLanglet requested a review from stof October 4, 2023 09:19

nicolas-grekas reviewed Oct 27, 2023

View reviewed changes

nicolas-grekas added DependencyInjection and removed Feature labels Oct 27, 2023

carsonbot changed the title ~~Introduce ServiceLoadingException to scope Container exceptions~~ [DependencyInjection] Introduce ServiceLoadingException to scope Container exceptions Oct 27, 2023

nicolas-grekas changed the title ~~[DependencyInjection] Introduce ServiceLoadingException to scope Container exceptions~~ [DependencyInjection] Remove inaccurate @throws on Container::get() Oct 27, 2023

nicolas-grekas approved these changes Oct 27, 2023

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Oct 27, 2023

[DependencyInjection] Remove inaccurate @throws on Container::get()

5edc2b1

nicolas-grekas force-pushed the serviceLoading branch from a3d6359 to 5edc2b1 Compare October 30, 2023 13:58

nicolas-grekas merged commit 61023a7 into symfony:6.4 Oct 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[DependencyInjection] Remove inaccurate `@throws` on `Container::get()` #51835

[DependencyInjection] Remove inaccurate `@throws` on `Container::get()` #51835

Uh oh!

VincentLanglet commented Oct 4, 2023 •

edited by nicolas-grekas

Loading

Uh oh!

derrabus commented Oct 4, 2023

Uh oh!

VincentLanglet commented Oct 4, 2023

Uh oh!

Uh oh!

VincentLanglet commented Oct 9, 2023

Uh oh!

nicolas-grekas Oct 27, 2023

Uh oh!

VincentLanglet Oct 27, 2023 •

edited

Loading

Uh oh!

nicolas-grekas Oct 27, 2023

Uh oh!

VincentLanglet Oct 27, 2023

Uh oh!

nicolas-grekas commented Oct 30, 2023

Uh oh!

Uh oh!

	public function testGetThrowsException()
	{
	$this->expectException(\Exception::class);
	$this->expectExceptionMessage('Something went terribly wrong!');
	$c = new ProjectServiceContainer();

	try {
	$c->get('throw_exception');
	} catch (\Exception $e) {
	// Do nothing.
	}

	// Retry, to make sure that get*Service() will be called.
	$c->get('throw_exception');
	}

	protected function getThrowExceptionService()
	{
	throw new \Exception('Something went terribly wrong!');
	}

Uh oh!

[DependencyInjection] Remove inaccurate @throws on Container::get() #51835

[DependencyInjection] Remove inaccurate @throws on Container::get() #51835

Uh oh!

Conversation

VincentLanglet commented Oct 4, 2023 • edited by nicolas-grekas Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

derrabus commented Oct 4, 2023

Uh oh!

VincentLanglet commented Oct 4, 2023

Uh oh!

Uh oh!

VincentLanglet commented Oct 9, 2023

Uh oh!

nicolas-grekas Oct 27, 2023

Choose a reason for hiding this comment

Uh oh!

VincentLanglet Oct 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Oct 27, 2023

Choose a reason for hiding this comment

Uh oh!

VincentLanglet Oct 27, 2023

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Oct 30, 2023

Uh oh!

Uh oh!

[DependencyInjection] Remove inaccurate `@throws` on `Container::get()` #51835

[DependencyInjection] Remove inaccurate `@throws` on `Container::get()` #51835

VincentLanglet commented Oct 4, 2023 •

edited by nicolas-grekas

Loading

VincentLanglet Oct 27, 2023 •

edited

Loading