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

issue 324: change parameter name from options to flags #376

Conversation

MarcinGladkowski
Copy link
Contributor

@MarcinGladkowski MarcinGladkowski commented Aug 4, 2022

No description provided.

@MarcinGladkowski MarcinGladkowski mentioned this pull request Aug 4, 2022
@dbrekelmans dbrekelmans merged commit a047dee into thecodingmachine:master Aug 4, 2022
2 checks passed
@danilopolani
Copy link

danilopolani commented Aug 4, 2022

Hey @MarcinGladkowski and @dbrekelmans , this was a breaking change (both param renamed and function removed), but has been marked as a minor update. For example, it has broken the package https://github.com/jessarcher/laravel-castable-data-transfer-object here.

Is there any chance we can rollback this?

@dbrekelmans
Copy link
Collaborator

dbrekelmans commented Aug 4, 2022

Hi @danilopolani. Thanks for the report! This is a though one, because it's hard to support BC for parameter names. This was one of the issues brought up in the internals discussion on the named arguments RFC. Some big open-source projects specifically state that they don't support named arguments as part of their BC promise.

While this library doesn't have a written policy on BC, I personally think it's a lot of effort to support this, probably more than it's worth. If you have strong opinions about this, please feel free to continue the discussion here!

@danilopolani
Copy link

danilopolani commented Aug 4, 2022

Hi @danilopolani. Thanks for the report! This is a though one, because it's hard to support BC for parameter names. This was one of the issues brought up in the internals discussion on the named arguments RFC. Some big open-source projects specifically state that they don't support named arguments as part of their BC promise.

While this library doesn't have a written policy on BC, I personally think it's a lot of effort to support this, probably more than it's worth. If you have strong opinions about this, please feel free to continue the discussion here!

Definitely a tough one, thank you for the explanation 😄 Gonna submit a PR to that project to fix this behaviour, in the meanwhile I've solved by requiring a fixed (old) version of this package.

Thank you again!

*
* @return mixed
* @throws JsonException if the JSON cannot be decoded.
* @link http://www.php.net/manual/en/function.json-decode.php
*/
function json_decode(string $json, bool $assoc = false, int $depth = 512, int $options = 0)
function json_decode(string $json, bool $assoc = false, int $depth = 512, int $flags = 0): mixed
Copy link

@shadowhand shadowhand Aug 5, 2022

Choose a reason for hiding this comment

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

Just as a note, this could be considered a breaking change in PHP 8.x because anyone using the named argument will be faced with a breaking change in a bugfix package release.

EDIT: I see this was reported above. I feel pretty strongly about this issue, especially for this package which aims to have parity with PHP functions of the same name. I am 100% for ensuring that this package stays in alignment with PHP current in terms of parameter names. However, I also think there is very little cost to publishing a new minor, or even major, release when parameter names change. If this change had bumped to 2.3.0 instead of 2.2.3, would anything bad have happened? I doubt it.

* @throws OpensslException
*
*/
function openssl_random_pseudo_bytes(int $length, ?bool &$strong_result = null): string

Choose a reason for hiding this comment

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

I just ran into this breaking change when upgrading safe. Shouldn't this method have been moved to the deprecated space instead of just being removed? As i randomly ran into an error after doing a minor update, without a deprecation notice prior.

Choose a reason for hiding this comment

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

I'm up for making a PR is that is preferable btw

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.

None yet

5 participants