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

First time use of Toggle is broken in a structured field #1950

Closed
GiantCrocodile opened this issue Jul 26, 2019 · 23 comments

Comments

@GiantCrocodile
Copy link

commented Jul 26, 2019

Describe the bug
Given this structured field:

      sources:
        when:
          has_sources: true
        type: structure
        fields:
          is_url:
            type: toggle
            default: false
            label: URL?
          name_url:
            type: url
            required: false
            when:
              is_url: true
          name_other:
            type: text
            required: false
            when:
              is_url: false

One of both fields is missing on first time:
grafik

To Reproduce
You have to toggle at least once to make sure that one of the both fields (name_url or name_other) is shown. I think this is not expected behavior or designed like this on purpose.

Expected behavior
Either default triggers or not even default is required to see the field which is enabled when the toggle is off.

grafik

Kirby Version
3.2.2

Desktop (please complete the following information):
Windows 10, latest FireFox

@texnixe

This comment has been minimized.

Copy link
Contributor

commented Jul 28, 2019

Yes, I can confirm this. Doesn't happen when using a radio field instead of the toggle field, for example.

@GiantCrocodile

This comment has been minimized.

Copy link
Author

commented Jul 28, 2019

To extend what I already mentioned here, even if it may be obvious for everyone: This should be fixed and then tested regarding required: true too. A required toggle field should be always in a valid state (because a toggle knows no invalid state).

@bastianallgeier bastianallgeier added this to the 3.2.5 milestone Sep 16, 2019
@bastianallgeier

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

@GiantCrocodile

This comment has been minimized.

Copy link
Author

commented Sep 16, 2019

This was an usability showstopper for me. Thank you @bastianallgeier and @afbora! You are an excellent community member @afbora! I'm glad I bought a license.

@GiantCrocodile

This comment has been minimized.

Copy link
Author

commented Sep 17, 2019

This fix wasn't complete: The default value is respected but the when is broken. URL? is on but name_other is still hidden. Also it seems like the default has issues with saving now. For example when I load an existing page and click on the toggle, the toggle is not switched and the save is still active. Creating a new page leads to a new save bar (the bar at the bottom of the page when you trigger changes) where revert and save are both grayed out.

Seems like side effects are occuring due to this fix or some other fix of Kirby 3.2.5-rc.1.

@afbora @bastianallgeier

@distantnative distantnative reopened this Sep 17, 2019
@afbora

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2019

I have tested now, yes not working.

But I'm sure it wasn't this commit.
Because it works when I put the same update in 3.2.4.

I tracked down:
In 3.2.4, field.default variable true or false
In 3.2.5, field.default variable 1 or `` (empty for false)

An update on the method that reads the default values seems to have caused it.

@afbora

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2019

Compare versions on section field with dump

if ($section = $this->page("notes/exploring-the-universe/")->blueprint()->section("main-col-0-fields")) {
    var_dump($section);
}

v3.2.4

["is_url"]=>
    array(13) {
      ["default"]=>
      bool(false)
      ["value"]=>
      bool(false)
}

v3.2.5

["is_url"]=>
    array(13) {
      ["default"]=>
      string(0) ""
      ["value"]=>
      string(0) ""
}
@afbora

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2019

@distantnative I found the source of causing commit:

9b94c96#diff-24db442c2898568e735f9518228da0bbR196-R199

We make sure that the toString() method does not cause any other errors.

@distantnative

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2019

My current problem is, that from reading the code I get why toString should be the source of th eproblem, but I couldn't get any unit tests set up that would fail when calling toString with e.g. false. Which is weird.

@afbora

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2019

Hmm, in particular, do we have a test with a boolean default value?

@distantnative

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2019

No, we don't. But I was writing one locally and it didn't fail - while I would expect it to fail :/

@afbora

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2019

Can you share your test? Maybe there's a point we missed.

@afbora

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2019

I have an idea of how to solve the issue.
I think this is actually what it should be.
We can overwrite the computed default method and revert simple return on props default method in the toggle field like that:

'props' => [
    'default' => function ($default = null) {
        return $default;
    }
],
'computed' => [
    'default' => function () {
        return $this->toBool($this->default);
    },
]

https://github.com/getkirby/kirby/blob/release/3.2.5/config/fields/toggle.php#L37-L45

Tested on RC and works for me 🎉

Just like we did in other fields.
For example, when entering an array in the default value for select field, it wasn't supposed to work with this commit, but it works because default method was overwritten.

https://github.com/getkirby/kirby/blob/release/3.2.5/config/fields/radio.php#L22-L23

Of course, we should check other fields to avoid such errors.

@bastianallgeier

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2019

@distantnative I know why you didn't manage to write a failing test. You probably used assertEquals, right? I just found out that it is not strict 😱. I created a failing test like this:

    public function testDefaultValue()
    {
        $field = $this->field('toggle', [
            'default' => true
        ]);

        $this->assertTrue($field->default() === true);
    }

@afbora your fix looks good!

@bastianallgeier

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2019

I'm a bit concerned that this commit will cause a lot more damage in places that we don't realise yet. I think we can must either write tests for all default values for all fields or revert it for now.

@afbora

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2019

I think it should be done for all computed methods.

https://github.com/getkirby/kirby/blob/release/3.2.5/src/Form/Field.php#L184-L209

bastianallgeier added a commit that referenced this issue Sep 18, 2019
@distantnative

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2019

@bastianallgeier Omg that's exactly why! How is a test called equals not strict?!

My train of thought for moving forward:

  • reverting could create quite a mess, not a big fan of that thought
  • while there have been a few computed methods with toString added, default is the only one of them that is not exclusively a string. Or am I wrong here?
  • We can fix this by altering the dafult computed part to:
'default' => function () {
    if ($this->default === null) {
        return;
    }

    if (is_string($this->default) === false) {
        return $this->default;
    }

    return $this->model()->toString($this->default);
},
@bastianallgeier

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2019

The string check is good and in combination with tests it should be solid.

@bastianallgeier

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2019

@lukasbestle

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2019

Omg that's exactly why! How is a test called equals not strict?!

@bastianallgeier @distantnative I also found out about this two days ago. There is an assertSame() method that apparently compares strictly (I mean WTF, they sound like exact aliases but they aren't). Maybe we should switch over to that over time.

@bastianallgeier

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2019

@lukasbestle @distantnative it's pretty insane. I really wonder how much cases we miss because we all believe it's strickt.

@GiantCrocodile

This comment has been minimized.

Copy link
Author

commented Sep 19, 2019

Sounds like you should create an internal todo or some issue on Github to track the move from equals to same. @bastianallgeier

@lukasbestle

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2019

I have a todo list for test refactoring/more unit tests and I've added this to it. I will also migrate the tests I have already refactored to it wherever possible (in some places we do actually need the non-strict behavior).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants
You can’t perform that action at this time.