Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
tinus opened this issue Aug 17, 2021 · 13 comments
Assignees
Labels
type: bug 🐛 Is a bug; fixes a bug type: stale 💤 Will be closed soon because there was no recent activity

Comments

@tinus
Copy link

tinus commented Aug 17, 2021

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.

@afbora afbora added type: bug 🐛 Is a bug; fixes a bug type: regression 🚨 Is a regression between versions labels Aug 19, 2021
@afbora
Copy link
Member

afbora commented Aug 19, 2021

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

@tinus
Copy link
Author

tinus commented Aug 21, 2021

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
Copy link
Member

@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
Copy link
Author

tinus commented Aug 22, 2021

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

@lukasbestle
Copy link
Member

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

@bastianallgeier
Copy link
Member

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

@tinus
Copy link
Author

tinus commented Sep 9, 2021

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
Copy link
Member

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
Copy link
Author

tinus commented Sep 10, 2021

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
Copy link
Author

tinus commented Sep 10, 2021

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
Copy link
Author

tinus commented Sep 10, 2021

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
Copy link
Author

tinus commented Sep 14, 2021

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.

@distantnative distantnative removed the type: regression 🚨 Is a regression between versions label Nov 16, 2021
@stale
Copy link

stale bot commented Aug 10, 2022

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 stale bot added the type: stale 💤 Will be closed soon because there was no recent activity label Aug 10, 2022
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug 🐛 Is a bug; fixes a bug type: stale 💤 Will be closed soon because there was no recent activity
Projects
None yet
Development

No branches or pull requests

5 participants