kirby icon indicating copy to clipboard operation
kirby copied to clipboard

$page->update validation doesn't throw exception and doesn't listen to validation flag.

Open tinus opened this issue 3 years ago • 13 comments

Describe the bug
When using the page's update function from a controller the input is validated even with the validate flag set to false.

No exception is thrown even when the validate flag is true.

The page does get updated but the field values that aren't correct are not stored, the fields are empty

To Reproduce
Steps to reproduce the behavior:

  1. Install starterkit.
  2. add 2 date fields to the album.yml blueprint.
      date_1:
        type: date
      date_2:
        type: date 
  1. add update code to the controller in album.php
    try {
      $page->update(['date_1'=>'NAN'],null,false);
    } catch (Exception $e) {
      echo 'Caught exception at date_1: ',  $e->getMessage(), "\n";
      die();
    }

    try {
      $page->update(['date_2'=>'NAN'],null,true);
    } catch (Exception $e) {
      echo 'Caught exception at date_2: ',  $e->getMessage(), "\n";
      die();
    }
  1. open any album page.
  2. open the content file to see that the file has been updated but no data is recorded:
----

Date-1: 

----

Date-2: 

Expected behavior

I expect date_1 to be filled with "NAN" since validation is set to false. I expect the update for date_2 to throw an exception.

Kirby Version
3.3.6

Additional context

I am currently moving a website from kirby2 to kirby3, the site has a lot of frontend editing and a number of field types do not work in the new version (as in the data stored is not compatible). It is not just the date fields but other field types too.

tinus avatar Aug 17 '21 13:08 tinus

I've tracked down the issue. Since the ->toDatetime() method returns null if the date is invalid, the input never enters validation.

afbora avatar Aug 19 '21 22:08 afbora

The same happens with the pages field:

If I add this field to the blueprint

      linked:
        type: pages

then this in the controller will work:

    try {
      $page->update(['linked'=>"
        - photography/sky
        - photography/ocean
      "],null,true);
    } catch (Exception $e) {
      echo 'Caught exception at linked: ',  $e->getMessage(), "\n";
      die();
    }

And will result in the correct date being entered in the content file.

This however:

    try {
      $page->update(['linked'=>"
        - photography/sky
        - photography/notapage
      "],null,true);
    } catch (Exception $e) {
      echo 'Caught exception at linked: ',  $e->getMessage(), "\n";
      die();
    }

Will result in an empty field and no exception thrown. I have not yet managed to get update to throw any exception no matter what I send to it.

tinus avatar Aug 21 '21 14:08 tinus

@tinus Is that $page a draft by any chance? Because PageActions::update() forces $validate = false for drafts. That still doesn't explain why invalid data is silently dropped even with $validate === false, but it could be an explanation why you don't get an exception.

lukasbestle avatar Aug 22 '21 12:08 lukasbestle

No, all listed pages. I haven't tried it with drafts.

tinus avatar Aug 22 '21 13:08 tinus

OK, that's even more strange then. Unfortunately I don't fully grasp the validation logic. I think it's something for @bastianallgeier.

lukasbestle avatar Sep 04 '21 15:09 lukasbestle

@tinus what does get stored in your last example with the non-existing page?

bastianallgeier avatar Sep 09 '21 14:09 bastianallgeier

this:

Linked:

- photography/sky

I thought it was empty but apparently it does save the valid page. (sorry about that) It just ignores the invalid page. The same thing gets stored when validate is false.

That means that the problem I am having with my own code is that the pages somehow don't "exist". That is very possible because I haven't imported all the content from the kirby2 site yet.

tinus avatar Sep 09 '21 17:09 tinus

Yes, that's the expected behaviour of the pages field. It simply ignores invalid entries. When you rely on validation to throw errors about invalid pages, I totally see why this feels like a bug. But we cannot solve this in a different way to be honest. Otherwise we would have to build an interface in the page picker to deselect or discard broken pages.

bastianallgeier avatar Sep 10 '21 07:09 bastianallgeier

But if validation is set to false it does the same. I am not looking for an error message, I am having trouble with data disappearing because it doesn't validate even though validate is set to false.

tinus avatar Sep 10 '21 11:09 tinus

I tried the same for a range field:

    try {
      $page->update(['range_field'=>"
        not a valid value
      "],null,true);
    } catch (Exception $e) {
      echo 'Caught exception at linked: ',  $e->getMessage(), "\n";
      die();
    }

    try {
      $page->update(['range_field'=>"
        not a valid value
      "],null,false);
    } catch (Exception $e) {
      echo 'Caught exception at linked: ',  $e->getMessage(), "\n";
      die();
    }

No matter if the validate flag is set to true or false, no error is reported and the value is set to 0.

tinus avatar Sep 10 '21 13:09 tinus

And the same goes for an email field:

    try {
      $page->update(['email_field'=>"
        not a valid value
      "],null,true);
    } catch (Exception $e) {
      echo 'Caught exception at linked: ',  $e->getMessage(), "\n";
      die();
    }

    try {
      $page->update(['email_field'=>"
        not a valid value
      "],null,false);
    } catch (Exception $e) {
      echo 'Caught exception at linked: ',  $e->getMessage(), "\n";
      die();
    }

No error is reported when validate is set to true and even when validate is set to false the data gets set to an empty string.

tinus avatar Sep 10 '21 13:09 tinus

I have tried to figure out where the data gets removed and I can see that by the time Kirby\Form\Field validate method is called the data is already gone. I am having trouble understanding the Kirby\Toolkit\Component applyProps and applyComputed methods so I have to give up there.

tinus avatar Sep 14 '21 18:09 tinus

This issue has been automatically marked as stale because it has not had recent activity. This is for us to prioritize issues that are still relevant to our community. It will be closed if no further activity occurs within the next 14 days. If this issue is still relevant to you, please leave a comment.

stale[bot] avatar Aug 10 '22 10:08 stale[bot]