Page MenuHomekolab.org

User profile (Bifrost#T331493)
ClosedPublic

Authored by machniak on Feb 25 2020, 9:48 AM.

Diff Detail

Repository
rK kolab
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

machniak requested review of this revision.Feb 25 2020, 9:48 AM
machniak created this revision.
vanmeeuwen requested changes to this revision.Feb 25 2020, 10:13 AM
vanmeeuwen added inline comments.
src/app/Http/Controllers/API/UsersController.php
293

Can these both just be if (!empty())?

src/app/Http/Kernel.php
22 ↗(On Diff #2341)

Why is this disabled?

This revision now requires changes to proceed.Feb 25 2020, 10:13 AM
vanmeeuwen added inline comments.Feb 25 2020, 10:16 AM
src/app/Http/Controllers/API/UsersController.php
267

phpstan tells me update() may also return void... which is from the abort() returns?

machniak added inline comments.Feb 25 2020, 10:35 AM
src/app/Http/Controllers/API/UsersController.php
267

I'll fix all API controllers code to always respond with JSON, and not use abort().

293

Could work untill we want to support passwords like "000000000" and other "nullable" strings.

src/app/Http/Kernel.php
22 ↗(On Diff #2341)

I found it problematic with input validation. Not impossible, but harder to handle . For example, user settings cannot be NULL at the moment, so to reset (set to an empty value) a setting we just POST empty strings. If they are converted to null, we need to add more code to detect that.

machniak updated this revision to Diff 2359.Feb 25 2020, 12:52 PM
  • Re-enable ConvertEmptyStringsToNull middleware
  • Always return JSON responses from API, even on 403/404 errors
machniak updated this revision to Diff 2365.Feb 25 2020, 1:26 PM
  • Make the code less ugly
vanmeeuwen accepted this revision.Feb 25 2020, 1:27 PM
This revision is now accepted and ready to land.Feb 25 2020, 1:27 PM
This revision was automatically updated to reflect the committed changes.