Use new params.expect syntax instead of params.require#573
Merged
dhh merged 1 commit intorails:mainfrom Sep 15, 2024
Merged
Conversation
4 tasks
|
@jeromedalbert add a Changelog entry, please |
martinemde
reviewed
Sep 14, 2024
martinemde
left a comment
martinemde
left a comment
There was a problem hiding this comment.
Thanks for following up with this. I think we should not change the fetch cases. Otherwise looks good (with changelog as mentioned).
There was a problem hiding this comment.
If we replace params.fetch like this it changes the behavior. Now, even though there are no attributes expected, you must send at least one attribute or you'll get a 400 error.
I suggest changing only the case with attributes and leaving this one alone.
Contributor
Author
There was a problem hiding this comment.
@martinemde This makes sense, should the same be reverted from your PR on the Rails side?
and
There was a problem hiding this comment.
You're exactly right. I submitted rails/rails#52932 to fix it. Thanks for noticing that I was in violation of my own recommendation. Thanks!
There was a problem hiding this comment.
This was the purpose of params.allow. Please someone from rails core let me know if that's worth a second, more focused attempt and I can extract the original code back out.
jeromedalbert
force-pushed
the
params-expect-syntax
branch
from
e54ef73 to
5d804e4
Compare
4 tasks
Contributor
Author
|
@MatheusRich This project does not seem to have a Changelog file. 😅 After looking at this: Line 26 in acf44b8 It looks like changelogs are made directly from GitHub at release time: https://github.com/rails/jbuilder/releases So I guess I'm good on my end, but let me know if I missed something. |
martinemde
added a commit
to martinemde/rails
that referenced
this pull request
Sep 15, 2024
The conversions have the unintended consequence of requiring params when none are expected on the default generated templates. Originally pointed out in rails/jbuilder#573.
martinemde
added a commit
to martinemde/rails
that referenced
this pull request
Sep 15, 2024
The conversions have the unintended consequence of requiring params when none are expected on the default generated templates. Adds a missing test for the api controller generation. Originally pointed out in rails/jbuilder#573.
martinemde
added a commit
to martinemde/rails
that referenced
this pull request
Sep 15, 2024
The conversions have the unintended consequence of requiring params when none are expected. Ensure generated templates work as is. Adds a missing test for the api controller generation. Originally pointed out in rails/jbuilder#573.
derricklannaman
referenced
this pull request
in powerhome/power-web-development-interview
Sep 24, 2024
This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [jbuilder](https://redirect.github.com/rails/jbuilder) ([changelog](https://redirect.github.com/rails/jbuilder/releases/tag/v2.13.0)) | `2.12.0` -> `2.13.0` | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>rails/jbuilder (jbuilder)</summary> ### [`v2.13.0`](https://redirect.github.com/rails/jbuilder/releases/tag/v2.13.0) [Compare Source](https://redirect.github.com/rails/jbuilder/compare/v2.12.0...v2.13.0) #### What's Changed - Redirect to `@record` or path in controller generator by [@​jeromedalbert](https://redirect.github.com/jeromedalbert) in [https://github.com/rails/jbuilder/pull/569](https://redirect.github.com/rails/jbuilder/pull/569) - Return early from collection partial rendering if blank by [@​tylerjc](https://redirect.github.com/tylerjc) in [https://github.com/rails/jbuilder/pull/560](https://redirect.github.com/rails/jbuilder/pull/560) - Add missing ':see_other' status code in generated destroy controller method by [@​ldeld](https://redirect.github.com/ldeld) in [https://github.com/rails/jbuilder/pull/538](https://redirect.github.com/rails/jbuilder/pull/538) - Remove OpenStruct references from Jbuilder by [@​mtsmfm](https://redirect.github.com/mtsmfm) in [https://github.com/rails/jbuilder/pull/567](https://redirect.github.com/rails/jbuilder/pull/567) - Use new `params.expect` syntax instead of `params.require` by [@​jeromedalbert](https://redirect.github.com/jeromedalbert) in [https://github.com/rails/jbuilder/pull/573](https://redirect.github.com/rails/jbuilder/pull/573) #### New Contributors - [@​jeromedalbert](https://redirect.github.com/jeromedalbert) made their first contribution in [https://github.com/rails/jbuilder/pull/570](https://redirect.github.com/rails/jbuilder/pull/570) - [@​tylerjc](https://redirect.github.com/tylerjc) made their first contribution in [https://github.com/rails/jbuilder/pull/560](https://redirect.github.com/rails/jbuilder/pull/560) - [@​ldeld](https://redirect.github.com/ldeld) made their first contribution in [https://github.com/rails/jbuilder/pull/538](https://redirect.github.com/rails/jbuilder/pull/538) - [@​mtsmfm](https://redirect.github.com/mtsmfm) made their first contribution in [https://github.com/rails/jbuilder/pull/567](https://redirect.github.com/rails/jbuilder/pull/567) **Full Changelog**: rails/jbuilder@v2.12.0...v2.13.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR was generated by [Mend Renovate](https://mend.io/renovate/). View the [repository job log](https://developer.mend.io/github/powerhome/power-web-development-interview). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC43NC4xIiwidXBkYXRlZEluVmVyIjoiMzguNzQuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOltdfQ==--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
DanielaVelasquez
pushed a commit
to DanielaVelasquez/rails
that referenced
this pull request
Oct 3, 2024
The conversions have the unintended consequence of requiring params when none are expected. Ensure generated templates work as is. Adds a missing test for the api controller generation. Originally pointed out in rails/jbuilder#573.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Problem
PR rails/rails#51674 updated controller generator templates in the Rails codebase. But, as DHH explained, we need to do the same update to templates in jbuilder, because they are used by default for new Rails apps.
Solution
Port the changes from rails/rails#51674 to this codebase, only for Rails versions 8.0+:
Change templates to use
params.expect(post: [ :title, ... ])instead ofparams.require(:post).permit(:title, ...).Notice the spaces around
[and]in order to be compliant withrubocop-rails-omakaserules, which are enabled by default for new Rails apps.Change templates to use
@post = params.expect(:id)instead of@post = params[:id].cc @martinemde