core icon indicating copy to clipboard operation
core copied to clipboard

WriteListener is called on Get operations

Open Fabrn opened this issue 1 year ago • 11 comments

API Platform version(s) affected: 3.3 +

Description

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.

How to reproduce

Create a Get operation like so :

new Get(
    uriTemplate: '/something/by_something/{not_id}',
    controller: SomethingController::class,
    output: SomethingOutput::class,
    read: false
)

Possible Solution

Sadly I don't come with a simple fix BUT I figured out that the persist attribute is true after being resolved in WriteListener. This is caused by the fact that the Request attribute _api_persist is not defined and defined as true by default, which is a shame since writing in a Get operation looks kind of weird.

Additional Context

This exact setup works perfectly using 3.2.x

Also I figured out that on a simple operation like :

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

The WriteProcessor is also called which makes no sense.

Fabrn avatar Aug 05 '24 13:08 Fabrn

Could you propose a fix? Indeed the condition should be:

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

soyuka avatar Aug 09 '24 06:08 soyuka

Of course, I will try to do it asap 🙂

Fabrn avatar Aug 09 '24 11:08 Fabrn

Alright finally had the time to check into it, and it might sound stupid, but I'm having some trouble validating my pull request. I did follow the steps explained by the docs about contributing but I cannot run PHPUnit on my fork since I'm getting the following message :

PHPUnit is not installed. Please run ./vendor/bin/simple-phpunit to install it

So I ran the Simple PHPUnit, but I'm not able to install PHPUnit since I get the following dependency error :

Updating dependencies
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - phpunit/php-code-coverage 9.2.31 requires nikic/php-parser ^4.18 || ^5.0 -> found nikic/php-parser[v4.18.0, v4.19.0, v4.19.1, v5.0.0, v5.0.1, v5.0.2, v5.1.0] but it conflicts with your root composer.json require (4.16).
    - phpunit/php-code-coverage 9.2.32 requires nikic/php-parser ^4.19.1 || ^5.1.0 -> found nikic/php-parser[v4.19.1, v5.1.0] but it conflicts with your root composer.json require (4.16).
    - Root composer.json requires phpunit/php-code-coverage ^9.2.31 -> satisfiable by phpunit/php-code-coverage[9.2.31, 9.2.32].

Use the option --with-all-dependencies (-W) to allow upgrades, downgrades and removals for packages currently locked to specific versions.

I did not dive into it really yet but is it normal that I get this issue while trying to setup my fork ? Isn't there something wrong with the dependencies here ?

Thanks in advance for helping out 🙂

Fabrn avatar Aug 24 '24 17:08 Fabrn

Alright, I found out that the composer.json file generated by the Symfony PHPUnit Bridge was using the 4.6 version of nikic/php-parser because of the SYMFONY_PHPUNIT_REQUIRE variable defined in the phpunit.xml.dist file. Setting it to <env name="SYMFONY_PHPUNIT_REQUIRE" value="nikic/php-parser:^5.1"/> resolves the PHPUnit installation.

However, I don't even need to update anything, tests are failing because of the Doctrine MongoDB Bundle being incorrect on a method declaration.

Why do I struggle so much running tests ? What's wrong here ? 😆

Fabrn avatar Aug 28 '24 19:08 Fabrn

Hi, I guess you're using windows? Try to run only the test at ./tests/Symfony/EventListener/WriteListenerTest.php or is it WriteProcessorTest that you want? It's possible that for some tests you need the php mongodb extension.

I took a look, not sure where to look at. What's your value for use_symfony_listeners?

soyuka avatar Aug 29 '24 20:08 soyuka

Hi ! Yes sadly I am on Windows 😄 I didn't think about running only the WriteListenerTest since I wanted to be sure that everything is running but hey I guess this is fine. My tests are valid using the following bit of code updated :

$canWrite = $operation?->canWrite();

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

if (!$canWrite) {
    return;
}

Trying to run Behat but I am having the same mongodb trouble. I don't know if I can avoid this... Never used Behat before 🫤

Fabrn avatar Aug 29 '24 20:08 Fabrn

Just open a PR on the code change I'll handle the rest, thanks!

soyuka avatar Aug 30 '24 05:08 soyuka

Alright, pull request is ready. Thanks 😃

Fabrn avatar Aug 30 '24 20:08 Fabrn

try https://github.com/api-platform/core/issues/4891

g-ra avatar Sep 05 '24 11:09 g-ra

Hello. Thanks for your suggestion. I didn't try it but even though it may be working in our case, I don't think that it resolves the issue because of the fact that :

  • This use to work using version 3.2 and the fact that it does not work anymore is not expected
  • In my opinion, I shoud not have to set write: false on a Get operation

I do have a fix for my initial request already, but now I would like to fix the discovered issue since it should be working

Fabrn avatar Sep 08 '24 07:09 Fabrn

Well it's been a month now I guess we're leaving this uncorrected :(

Fabrn avatar Oct 08 '24 19:10 Fabrn