fix(Symfony): canWrite guessing in WriteListener

[Fabrn]

Q	A
Branch?	3.3
Tickets	Closes #6501

As explained in the issue, in version 3.3 : a Get operation using a Controller is always calling the WriteProcessor after the Controller is called, which causes an "Invalid identifier or value configuration" if you are using another identifier.

Example :

new Get(
    output: SomethingOutput::class,
    provider: SomethingProvider::class,
)

A simple operation like this calls the WriteProcessor.

[soyuka]

Suggested change

	$canWrite = $operation?->canWrite();
	if (null === $canWrite) {
	$canWrite = !$request->isMethodSafe();
	}
	if (!$canWrite) {
	return;
	}
	if (false == ($operation?->canWrite() ?? !$request->isMethodSafe())) {
	return;
	}

But shouldn't you put this line 146?

[Fabrn]

This bit of code seems fine to me 🙂 You could even use === instead of == here isn't it ? Since the expression between parenthesis will always return a boolean here.

But shouldn't you put this line 146?

I don't think so, since the issue is that we call the WriteProcessor about line 117 like so :

$data = $this->processor->process($controllerResult, $operation, $uriVariables, [
    'request' => $request,
    'uri_variables' => $uriVariables,
    'resource_class' => $operation->getClass(),
    'previous_data' => false === $operation->canRead() ? null : $request->attributes->get('previous_data'),
]);

We must leave the WriteListener before this call

[soyuka]

We already do this correctly line 104:

            if (null === $operation->canWrite()) {
                $operation = $operation->withWrite(!$request->isMethodSafe());
            }

[Fabrn]

Yeah. Holidays and a month went by, I had to setup a little project to remember what was going wrong here 😄

Initially, I wrote this issue since I get an "Invalid identifier value or configuration." error while running the following operation :

new Get(
    uriTemplate: '/users/by_identifier/{identifier}',
    controller: GetUserByIdentifier::class,
    read: false,
),

I then made my Controller which resolves the User like so :

#[AsController]
class GetUserByIdentifier
{
    public function __construct(
        private readonly UserRepository $userRepository,
    ) {}

    public function __invoke(string $identifier): User
    {
        return $this->userRepository->findOneBy(['identifiant' => $identifier]);
    }
}

The point of using read: false here is to prevent resource reading since we aren't using the Doctrine identifier here, and let the Controller do the job. However, after line 104, you get the following :

$uriVariables = $request->attributes->get('_api_uri_variables') ?? [];

if (!$uriVariables && !$operation instanceof Error && $operation instanceof HttpOperation) {
    try {
        $uriVariables = $this->getOperationUriVariables($operation, $request->attributes->all(), $operation->getClass());
    } catch (InvalidIdentifierException|InvalidUriVariableException $e) {
        throw new NotFoundHttpException('Invalid identifier value or configuration.', $e);
    }
}

This bit of code results in the "Invalid identifier value or configuration." error. If I add the bit of code discussed here, I dodge the URI variables resolving, and the call of the write processor, which allows me to have this working.

The point here is to not call the ReadListener with read: false because we would have got the issue and it would have been expected. But calling the WriteListener is not expected here and should not prevent my read operation from working.

Also, I personally think that we better dodge any more logic if possible. Resolving URI variables and calling the Processor for an operation that should not be handled for this Listener is too much

[soyuka]

I don't understand why you'd use controllers with API Platform ^^ use uriVariables: ['identifier']

Where is the getOperationUriVariables code you pasted here? you can also juste use write: false.

[Fabrn]

I don't understand why you'd use controllers with API Platform ^^ use uriVariables: ['identifier']

I confirm that my simple use case works fine using uriVariables: ['identifier']. I had a lack of knowledge about this.

However, since Api Platform does not prevent you from using controllers, they are a part of the bundle and the bundle must adapt and work with it. Since it works fine on 3.2 and always did, I don't know why we should stop supporting it. People with more complex behaviors may rely on controllers to work for their use case.

Also, if you guys don't want us to use controllers to work anymore, I think it's better to do it on purpose instead of explicitly letting a bug exist and leading to incorrect and confusing behavior.

Where is the getOperationUriVariables code you pasted here?

Version 3.3, WriteListener :

you can also juste use write: false

I don't agree with the fact that I must use write: false in a read operation. It is absurd