Skip to content

Conversation

brianstoop
Copy link

No description provided.

@@ -50,7 +50,7 @@ public function parse(object $object): self
$struct->deps = $deps;
$struct->parse($item->content, $deps);

if (isset($item->content->content[0]->meta->id)) {
if (isset($item->content->content) && is_array($item->content->content) && isset($item->content->content[0]->meta->id)) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this related to the rest of the changes?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, because the $item->content->content is not an array when there is only one enumeration.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it an object then?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, in my case it is the value of the enum. In the test you can see an example and there it is a string, see https://github.com/SMillerDev/phpdraft/pull/681/files#diff-b33b2099416ec2a729244bddbbf8f2f3125952aa5d11a6bdfb87f28eef655cbaR335-R337

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this could be a separate PR, because this change on it's own solve the issues we have right now. i also miss a test for this change

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my personal project I've done worse things for commit grouping. But whatever people do, the CI has to pass.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the reason i said it should be a separate PR, because then that could be merged earlier/separately

@SMillerDev
Copy link
Owner

@brianstoop will you fix the tests for this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants