Repository navigation
Save field values that have no validation rules - #561
Open
eastagiletracker wants to merge 1 commit into
Open
eastagiletracker wants to merge 1 commit into
eastagiletracker wants to merge 1 commit into
Conversation
Fieldtype::rules() returned no rule for a field with empty validation,
so its value was left out of the validated request data and dropped.
Optional settings (CORS lists, Supports Credentials, accepted file
types) never saved, and entry fields created without validation lost
their values. Fields stored in a column now fall back to "sometimes".
List items are saved as { _id, value } pairs, so list settings are now
turned into plain values before they override config (cors.paths etc.).
Also removes the stale "structures" block from the test stub config,
which merged into config('fusion.structures') and made SyncStructures
fail during install, erroring every test.
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
1 participant
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.
This PR proposes saving field values that have no validation rules, so optional settings such as the CORS lists and the Supports Credentials toggle actually persist (refs fusioncms/fusioncms#799). We include this PR work along with a full history of your repo at https://eastagiletracker.com/projects/736. You can sign in with your GitHub ID to claim ownership of the project.
What was wrong
Since #325,
Field::getValidationAttribute()returnsfalsefor an empty validation, andFieldtype::rules()then returns no rule at all for that field. A field without a rule is left out of$request->validated(), so its value is silently dropped: saving Settings -> CORS returns 200 but the paths, methods, origins and headers lists and the Supports Credentials toggle stay unchanged (the same goes forfiles.accepted_files), and matrix, single and taxonomy term fields created without validation (for example through the model factories) lose their values.CollectionTest,SingleTestandTermTestin this repo fail on exactly this.What changed
Fieldtype::rules()falls back tosometimesfor fields stored in a column, which is what these fields got before Asset Fieldtype Updates聽#325. Relationship and display-only fieldtypes keep returning no rule, since they are persisted from the request directly. Fields that have validation are unchanged.{ _id, value }pairs, while config such ascors.pathsexpects plain strings. Now that list settings save, a newFieldtype::getConfigValue()hook (a no-op by default, overridden inListFieldtype) turns them into plain values before they override config, in bothSetting::set()andSettingServiceProvider. Without it, a saved CORS list would hand nested arrays to the CORS middleware, which is the 500 described in CORS Settings do not have defaults聽fusioncms#802. This does not touch the List field's front end, so the separate "defaults don't show" part of #799 is still open. It also takes a different route from the open Settings list saves, accepted file types work as expected聽#407, which marks the CORS lists required.tests/stubs/laravel/config/fusion.phpstill had the old'structures' => ['forms' => 'Forms', ...]map. Deep-merged with the package'sstructureslist, it makesSyncStructuresread'Forms'['name']during install, so every test errored before running. This PR removes the stale block so the suite can run.Reproduction and verification
On
nightly(c2b116a) with PHP 8.2,vendor/bin/phpuniterrors on all 490 tests withTypeError: Cannot access offset of type string on stringatsrc/Console/Actions/SyncStructures.php:19. With the stub fixed,vendor/bin/phpunit --filter OptionalSettingTestfails at HEAD: afterPATCH api/settings/corswithcors_supports_credentials: truethe row still has"cors_supports_credentials": null, andsetting('cors.cors_paths')is still['api/*']after posting two paths. With this change all five new tests pass. If I revert only thegetConfigValue()calls,config('cors.paths')comes back as[['_id' => 'a1', 'value' => 'api/*'], ...]and the override test fails.I ran the full suite before and after (the lockfile's Laravel 8.40 does not boot on PHP 8.1+, so I updated
laravel/frameworkand testbench within their existing constraints locally;composer.lockis not part of this PR). Before: 490 tests with 55 failing or erroring. After: 495 tests with 52 failing or erroring, and none of them new.CollectionTest::a_user_with_permissions_can_create_a_new_collection_entry,SingleTest::a_user_with_permissions_can_update_a_singleandTermTest::a_user_with_permissions_can_create_a_new_termnow pass. The remaining failures were already failing before this change (backups, Composer service, the settings tests that post fewer fields than are now required, and more).How this was managed
We imported this repo's pull requests into a board and tracked this work there as a story: https://eastagiletracker.com/projects/736/stories/703958 on https://eastagiletracker.com/projects/736 (560 stories imported).
If you'd rather not receive contributions like this, reply
no-more-prson this pull request and we won't open any further ones on your repositories.Lawrence W. Sinclair
CEO / East Agile
linkedin.com/in/lwsinclair/
eastagile.com