Skip to content

[Serializer] error handling inconsistencies fixed in the serializer decoders#9586

Closed
rodrigodiez wants to merge 14 commits intosymfony:masterfrom
rodrigodiez:ticket_9393
Closed

[Serializer] error handling inconsistencies fixed in the serializer decoders#9586
rodrigodiez wants to merge 14 commits intosymfony:masterfrom
rodrigodiez:ticket_9393

Conversation

@rodrigodiez
Copy link
Copy Markdown

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets #9393
License MIT
Doc PR none

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Instead of using variable, you can use return:

if (!function_exists('json_last_error_msg')) {
    switch (json_last_error()) {
        default:
            return 'Unknown error';
    }
}

return json_last_error_msg();

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

updated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also, default should probably be the last case.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

updated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As you call return, the break is redundant here.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

updated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

don't you need also here the use statement?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yep. Sorry guys, first code PR, still learning

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

doing great! ;)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am learning too 👶

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

JsonDecode

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The deprecation message should not be the short description of the method. It should go with the @see tag (the first line of the phpdoc is special).

and you should remove the extra indentation space you added.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

You are right, updated all

@rodrigodiez
Copy link
Copy Markdown
Author

Seems ok to you guys?

@rodrigodiez
Copy link
Copy Markdown
Author

Ping

@fabpot
Copy link
Copy Markdown
Member

fabpot commented Dec 28, 2013

Closing this one in favor of #9876 as you seem to have removed your Symfony fork.

@fabpot fabpot closed this Dec 28, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants