Apply removeSensitiveData()'s formatted return in services() - #201
Merged
Conversation
services() reassigned the foreach loop variable instead of writing the result back into the \$services collection, so removeSensitiveData()'s formatted return (serializeApiResponse()'s key-sort/field-reordering) was discarded - sensitive fields stayed hidden only because makeHidden() mutates the model in place, but the response shape was inconsistent with every other endpoint in this controller.
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
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.
Summary
Found via a fresh
/code-reviewpass onapp/Http/Controllers/Api/ServicesController.php(issue #70).services()reassigned theforeachloop variable instead of writing the result back into the$servicescollection:Sensitive fields still ended up hidden only because
makeHidden()mutates the underlying model in place, butserializeApiResponse()'s key-sort/field-reordering (created_at/updated_at moved to the end, id/uuid/description/name prepended in that order) was discarded - the list endpoint's response shape was inconsistent with every other endpoint in this controller.Fix
->map()instead of aforeachthat discards its result, soremoveSensitiveData()'s actual return value is what gets serialized.Verification
TDD-proved:
formats the list endpoint response the same way removeSensitiveData() formats every other endpointassertsupdated_atis the last response key (serializeApiResponse()'s guarantee - raw model attribute order does neither the alphabetical sort nor the timestamp-to-end move). Confirmed failing against the pre-fix code (databaseslast instead) and passing after. Fulltests/v4/Feature/Api/suite passes (236 tests). Pint/PHPStan clean on the changed file.