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

Fix PHP error for parameter after optional parameter #1465

Merged
merged 9 commits into from
Apr 19, 2023

Conversation

afragen
Copy link
Contributor

@afragen afragen commented Jul 3, 2022

Fixes #1446

I'm not certain the changes to the unit tests are correct. I was unable to run phpunit according to the instruction for wp-env 😞

Copy link
Member

@ocean90 ocean90 left a comment

Choose a reason for hiding this comment

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

Moving the object ID isn't really an option as it provides a bad DX.

Instead I think we should introduce new functions which handle the global meta:

function gp_update_meta( $object_id, $meta_key, $meta_value, $type ) {}
function gp_update_global_meta( $meta_key, $meta_value, $type ) {}
function gp_delete_meta( $object_id, $meta_key, $meta_value, $type ) {}
function gp_delete_global_meta( $meta_key, $meta_value, $type ) {}

Thoughts?

@amieiro
Copy link
Member

amieiro commented Jan 11, 2023

Why not only remove the default value, because you always have to use a value when you call to these functions?

function gp_update_meta( $object_id, $meta_key, $meta_value, $type, $global = false ) {
function gp_delete_meta( $object_id, $meta_key, $meta_value, $type, $global = false ) {

These changes pass the tests. I have just checked in my machine.

@akirk
Copy link
Member

akirk commented Mar 8, 2023

I agree with Jesús, since it's not possible to make use of the default value (0) of the first parameter, let's just remove it. @afragen Shall we close this and create a new PR or would you be willing to update it?

@afragen
Copy link
Contributor Author

afragen commented Mar 8, 2023

I should be able to update the PR for that.

Copy link
Member

@akirk akirk left a comment

Choose a reason for hiding this comment

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

Thank you! Looks good to me!

@amieiro amieiro enabled auto-merge (squash) April 12, 2023 12:27
@amieiro
Copy link
Member

amieiro commented Apr 12, 2023

@ocean90 is it ok if I dismiss your review, to merge this PR?

@amieiro amieiro dismissed ocean90’s stale review April 19, 2023 08:15

The proposed solution takes a new approach, so this changes requested don't apply.

@amieiro amieiro merged commit 38a6baf into GlotPress:develop Apr 19, 2023
@afragen afragen deleted the fix-meta branch April 19, 2023 14:35
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.

PHP 8 issues with function parameter order
4 participants