getkirby / kirby

Kirby's core application folder
https://getkirby.com
Other
1.29k stars 168 forks source link

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

Closed tinus closed 2 years ago

tinus commented 3 years ago

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 
  3. 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.

afbora commented 3 years ago

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

tinus commented 3 years ago

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.

lukasbestle commented 3 years ago

@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.

tinus commented 3 years ago

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

lukasbestle commented 3 years ago

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

bastianallgeier commented 3 years ago

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

tinus commented 3 years ago

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.

bastianallgeier commented 3 years ago

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.

tinus commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.

stale[bot] commented 2 years ago

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.