hydephp / publications

Upcoming Publications Feature - HydePHP Extension
MIT License
0 stars 0 forks source link

Bug: Inconsistent front matter serialization for empty input #6

Open caendesilva opened 1 year ago

caendesilva commented 1 year ago

Minimal steps to reproduce

  1. Create test/schema.json with the following contents:
{
    "name": "test",
    "fields": [
        {
            "type": "string",
            "name": "foo"
        },
        {
            "type": "text",
            "name": "bar"
        }
    ]
}
  1. Run php hyde make:publication test
  2. Hit enter for the first field
  3. Immediately hit CTRL+D (and enter if needed) for the second field
  4. See result in created Markdown file

Actual result

On my machine (Windows)

---
__createdAt: 2023-02-20T18:37:15+00:00
bar: ''
---

## Write something awesome.

On Roberts machine (Linux)

---
__createdAt: 2023-02-20T19:04:12+00:00
bar: ''
---

## Write something awesome.

Expected result

The front matter should be one of the following:

Option A (Empty string)

---
__createdAt: xxx
foo: ''
bar: ''
---

Option B (No entries at all)

---
__createdAt: xxx
---
rgasch commented 1 year ago

OK, hitting enter (for the 1st field) and ctrl-d (for the 2nd field) I get this MD file:

---
__createdAt: 2023-02-20T19:04:12+00:00
bar: ''
---

## Write something awesome.

Which is correct. Interestingly enough, the CTRL-D worked.

I've tested the CTRL-D issue and if you add a field AFTER the text field and enter CTRL-D for the text field, then in breaks. I've used this schema file to test this:

{
    "name": "test",
    "fields": [
        {
            "type": "string",
            "name": "foo"
        },
        {
            "type": "text",
            "name": "bar"
        },
        {
            "type": "string",
            "name": "bars"
        }
    ]
}
caendesilva commented 1 year ago

So for the first part, thanks for confirming. Then we can rule out OS differences for this.

As for the second part, I cannot reproduce your crash (as shown in DM) on my Windows machine. I would guess that it has to do with OS inconsistencies with the EOT character.

Gonna to back and debug the first part

rgasch commented 1 year ago

OK, thanks :-)

caendesilva commented 1 year ago

Gonna make a few posts here of me thinking out loud. Robert, you are my rubber ducky now. Feel free to ignore me!

Starting to narrow down the location of where this bug happens, and I can confirm I'm getting unexpected results with your schema file with the additional field. So the two problems may be related.

The CreatesNewPublicationPage::normalizeData method when fed with input (a,b,c, for each field respectively, receives this collection: (so far all good)

^ array:4 [
  "__createdAt" => DateTime @1676920405 {#468
    date: 2023-02-20 19:13:25.0 UTC (+00:00)
  }
  "foo" => "a"
  "bar" => "b"
  "bars" => "c"
]

Feeding empty inputs to everything gives me this however:

^ array:2 [
  "__createdAt" => DateTime @1676920579 {#464
    date: 2023-02-20 19:16:19.0 UTC (+00:00)
  }
  "bar" => ""
]

For some reason we are not getting all entries here (which we need if we want Option A)

So the issue is before this method is called.

caendesilva commented 1 year ago

The empty values are not at all passed to the CreatesNewPublicationPage constructor.

Okay, found the location in the MakePublicationCommand::collectFieldData method:

if (empty($fieldInput)) {
    $this->line("<fg=gray> > Skipping field $field->name</>");
} else {
    $this->fieldData->put($field->name, $fieldInput);
}

So we'll want to add an empty value here, question is, what do we add so that we cover all field types? What should we do for example if the type is a boolean?

https://github.com/hydephp/develop/blob/f4bac01ec66386338733676ae180d5ca17acf6e9/packages/publications/src/Commands/MakePublicationCommand.php#L110

caendesilva commented 1 year ago

So we'll want to add an empty value here, question is, what do we add so that we cover all field types? What should we do for example if the type is a boolean?

Actually, partially disregard that. We can add these on a per-type basis, for example:

protected function captureOtherFieldInput(PublicationFieldDefinition $field): ?PublicationFieldValue
{
    $selection = $this->askForFieldData($field->name, $field->getRules());
    if (empty($selection)) {
        return null; <- HERE
    }

    return new PublicationFieldValue($field->type, $selection);
}
rgasch commented 1 year ago

I see no real change with this fix applied to my repo:

---
__createdAt: 2023-02-20T19:41:46+00:00
bar: ''
---

## Write something awesome.
caendesilva commented 1 year ago

That's weird. I get this:

---
__createdAt: 2023-02-20T19:44:15+00:00
foo: ''
bar: ''
bars: ''
---

## Write something awesome.
caendesilva commented 1 year ago

Once we fix this, another question remains, should we keep the > Skipping field xxx output?