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

Add stateless ApiResource attribute #3436

Merged
merged 1 commit into from Sep 25, 2020

Conversation

mtarld
Copy link
Contributor

@mtarld mtarld commented Mar 9, 2020

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tickets
License MIT
Doc PR TODO

In Symfony 5.1, it will be possible to declare routes as stateless (symfony/symfony#35732)
For stateless routes, kernel will either throw an exception (debug enabled) or log a warning (debug disabled) on session usage.

As API Platform shouldn't use session (according to REST architecture constraints), this PR proposes to add an ApiResource attribute in order to declare related routes as stateless: stateless

false will be the default in order to prevent BC break (and we could set it at true in the next major)

One question still remains. API Plateform could be on top of older version of Symfony (4.x, 5.0.x).
How to handle it ?

  • Throw an exception saying that this feature isn't supported for that Symfony's version
  • Log a warning saying that this feature isn't supported for that Symfony's version
  • Do nothing (which is dangerous IMO)

WDYT ?

@mtarld mtarld force-pushed the feature/stateless branch 2 times, most recently from cc70388 to d38dad2 Compare March 9, 2020 19:11
@mtarld mtarld changed the title Feature/stateless Add stateless ApiResource attribute Mar 9, 2020
@teohhanhui
Copy link
Contributor

I think the default cannot be false, but null. This allows us to trigger a deprecation error when it's null, so as to force the user to set it to true (or false). And then we can default to true in 3.0

WDYT @dunglas?

@teohhanhui
Copy link
Contributor

API Platform could be on top of older version of Symfony (4.x, 5.0.x).
How to handle it ?

We could detect the version of symfony/http-kernel by checking if Symfony\Component\HttpKernel\Exception\UnexpectedSessionUsageException exists. I don't think doing nothing is dangerous, as it's just an extra safety net that didn't exist before, so we use it if it exists, otherwise nothing has changed.

@mtarld
Copy link
Contributor Author

mtarld commented Mar 10, 2020

We could detect the version of symfony/http-kernel by checking if Symfony\Component\HttpKernel\Exception\UnexpectedSessionUsageException exists. I don't think doing nothing is dangerous, as it's just an extra safety net that didn't exist before, so we use it if it exists, otherwise nothing has changed.

We could as well use version_compare on Kernel::VERSION ?

Furthermore, what I meant is just telling the developer that he is using stateless but nothing will occur (as it's not handled by Symfony)

@teohhanhui
Copy link
Contributor

We could as well use version_compare on Kernel::VERSION ?

Uhh, yeah, I forgot this is the HttpKernel we're talking about. Lol...

telling the developer that he is using stateless but nothing will occur (as it's not handled by Symfony)

Personally, I think it's fine to just do nothing. It should be documented that this feature requires Symfony 5.1

@mtarld mtarld force-pushed the feature/stateless branch 6 times, most recently from 8579990 to e24f18e Compare March 12, 2020 16:09
@mtarld mtarld requested a review from teohhanhui March 13, 2020 09:09
@mtarld mtarld force-pushed the feature/stateless branch 2 times, most recently from ba015b2 to b33df4b Compare March 14, 2020 11:31
@mtarld
Copy link
Contributor Author

mtarld commented Mar 14, 2020

Ok, using the current approach, stateless priority will be handled as the following:

  1. Operation stateless attribute in attributes
  2. Operation stateless attribute
  3. Resource stateless attribute in attributes
  4. Resource stateless attribute
  5. Global defaults

Same goes for subresources.

@mtarld mtarld requested a review from teohhanhui March 18, 2020 07:53
@mtarld mtarld requested a review from dunglas April 2, 2020 16:13
Copy link
Contributor

@teohhanhui teohhanhui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing deprecation: #3436 (comment)

@mtarld mtarld force-pushed the feature/stateless branch 2 times, most recently from 3560f84 to ec288b4 Compare April 6, 2020 06:02
src/Bridge/Symfony/Routing/ApiLoader.php Outdated Show resolved Hide resolved
src/Bridge/Symfony/Routing/ApiLoader.php Outdated Show resolved Hide resolved
src/Bridge/Symfony/Routing/ApiLoader.php Outdated Show resolved Hide resolved
src/Bridge/Symfony/Routing/ApiLoader.php Outdated Show resolved Hide resolved
src/Operation/Factory/SubresourceOperationFactory.php Outdated Show resolved Hide resolved
@teohhanhui
Copy link
Contributor

I think there are some fundamental problems with the current implementation... This needs work.

@teohhanhui
Copy link
Contributor

And sorry for the earlier confusion, there's no explicit attributes key under each operation. The attributes are just directly specified.

@mtarld mtarld force-pushed the feature/stateless branch 3 times, most recently from 41374d0 to ccd2c90 Compare April 8, 2020 10:34
@mtarld
Copy link
Contributor Author

mtarld commented Apr 9, 2020

No worries! I should have been aware of that. The new implementation is much cleaner now, thanks.

@soyuka soyuka requested a review from teohhanhui April 15, 2020 15:29
Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you rebase please?

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.

None yet

4 participants