-
Notifications
You must be signed in to change notification settings - Fork 3
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
Move form definition retrieval out of controller constructors for proper access checking in other contexts #19
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…per access checking in other contexts
jensschuppe
added
bug
Something isn't working
enhancement
New feature or request
status:needs review
Code needs review and testing
labels
Jan 16, 2024
@dontub if you have time, could you have a look as well? Also, the PHPStan jobs are failing due to failing GitHub authentication … |
dontub
requested changes
Jan 18, 2024
modules/civiremote_event/src/Controller/RegisterFormController.php
Outdated
Show resolved
Hide resolved
modules/civiremote_event/src/Controller/RegisterFormController.php
Outdated
Show resolved
Hide resolved
modules/civiremote_event/src/Controller/RegisterFormController.php
Outdated
Show resolved
Hide resolved
modules/civiremote_event/src/Controller/RegisterFormController.php
Outdated
Show resolved
Hide resolved
dontub
requested changes
Jan 19, 2024
modules/civiremote_event/src/Controller/RegisterFormController.php
Outdated
Show resolved
Hide resolved
modules/civiremote_event/src/Controller/RegisterFormController.php
Outdated
Show resolved
Hide resolved
dontub
approved these changes
Jan 19, 2024
jensschuppe
added
status:fixed
The issue has been resolved (usually by committing/merging code)
and removed
status:needs review
Code needs review and testing
labels
Jan 19, 2024
This was referenced Feb 19, 2024
This introduced a regression causing all forms for all contexts (create, update, cancel) to use the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
bug
Something isn't working
enhancement
New feature or request
status:fixed
The issue has been resolved (usually by committing/merging code)
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #14. Replaces #15.
This is a more comprehensive approach to moving form definition retrieval out of controller constructors.
The
\Drupal\civiremote_event\Controller\RegisterFormController
class now responds to all registration-related routes (register, update, cancel) and includes a combined access check that can be used reliably outside of those route contexts (e.g. when validating a URL for displaying on another page).Also, retrieving the form definition is now done in a centralized controller method before handing things off to the respective form builders, which do not have to care for initializing things in their constructor anymore. This also prevents intermittent error messages be displayed for errors during route access checks.
@FeyP Would you be able to give this a try and leave a review?