Movatterモバイル変換

Copy link

Member

javiereguiluz left a comment•
edited
Loading

Choose a reason for hiding this comment

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

Matthieu, this is a good start, but I think we should also display an example of using this feature to add an explanation in some of the voter examples.

security.rst OutdatedShow resolvedHide resolved

security/voters.rst OutdatedShow resolvedHide resolved

carsonbot added Status: Needs Work and removed Status: Needs Review labels

94noni reviewed

		If you return ``true`` from ``supports()``, then this method is called. Your
		job is to return ``true`` to allow access and ``false`` to deny access.
		The ``$token`` can be used to find the current user object (if any). In this
		example, all of the complex business logic is included to determine access.
		The ``$token`` can be used to find the current user object (if any). The ``$vote``

Copy link

Contributor

Choose a reason for hiding this comment

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

perhaps document were this can be seen? or retrieved to be displayed?

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

I am not sure to understand your point here

Copy link

Contributor

Choose a reason for hiding this comment

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

I meant something like the workflow transition blocker message
is it possible to see this "vote message" somewhere and how ?

Copy link

Contributor

Choose a reason for hiding this comment

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

I agree with@94noni. Having a short section explaining how to retrieve and display the reasons to users would be a great addition. It would help developers make better use of the new Vote object in real-world applications.

Copy link

Member

javiereguiluzMay 23, 2025

Choose a reason for hiding this comment

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

I did some tweaks while merging, but we can do further tweaks in future PRs if we think this needs to be improved. Thanks.

94noni reviewed

security/voters.rstShow resolvedHide resolved

carsonbot added Status: Needs Review and removed Status: Needs Work labels

Mar 2, 2025

OskarStark reviewed

Mar 10, 2025

security.rst OutdatedShow resolvedHide resolved

OskarStark reviewed

Mar 10, 2025

security/voters.rst OutdatedShow resolvedHide resolved

OskarStark reviewed

Mar 10, 2025

security/voters.rst OutdatedShow resolvedHide resolved

OskarStark requested a review fromjaviereguiluz

March 10, 2025 17:00

javiereguiluz reviewed

May 9, 2025


		if (!$user instanceof User) {
		// the user must be logged in; if not, deny access
		$vote->reasons[] = 'The user is not logged in.';

Copy link

Member

javiereguiluzMay 9, 2025

Choose a reason for hiding this comment

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

I think we should instead use theaddReason() method.

$vote?->addReason('The user is not logged in.');

nicolas-grekas reviewed

May 10, 2025

		{
		$user = $token->getUser();
		$vote ??= new Vote();

Copy link

Member

nicolas-grekasMay 10, 2025

Choose a reason for hiding this comment

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

should be removed while accounting for@javiereguiluz's comment:

Suggested change

$vote ??= new Vote();

Copy link

Member

javiereguiluzMay 23, 2025

Choose a reason for hiding this comment

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

Fixed while merging. Thanks.

nicolas-grekas reviewed

May 10, 2025