Skip to content

Remove typecasting to prevent fatal when $screen_id is null. #34734

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.

Sign up for GitHub

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

Jump to bottom
Merged
Konamiman merged 3 commits into from
Sep 20, 2022
Merged

Remove typecasting to prevent fatal when $screen_id is null. #34734

Konamiman merged 3 commits into from
Sep 20, 2022

Conversation

@vedanshujain
Copy link
Contributor

@vedanshujain vedanshujain commented Sep 19, 2022 •
edited
Loading

All Submissions:

Changes proposed in this Pull Request:

Closes #34725

We added a function is_order_meta_box_screen which as string type definition as one of its param. From PHP 8.1, if null is passed as the screen_id, it's not typecasted, but instead a fatal is thrown. This PR removes the type definition to accomodate null screen ID.

How to test the changes in this Pull Request:

  1. Start with a WooCommerce site with PHP 8.1.
  2. Install a plugin that adds a new screen in admin, such as SEOPress
  3. Start the configuration wizard for the plugin. Without this PR you will see a fatal like so: Uncaught TypeError: WC_Admin_Assets::is_order_meta_box_screen(): Argum. With this PR, it should be resolved and setup screen with load as usual.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully run tests with your changes locally?
  • Have you created a changelog file for each project being changed, ie pnpm changelog add --filter=<project>?

FOR PR REVIEWER ONLY:

  • I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

@github-actions github-actions bot added focus: react admin plugin: woocommerce Issues related to the WooCommerce Core plugin. labels Sep 19, 2022
@github-actions github-actions bot added release: highlight Issues that have a high user impact and need to be discussed/paid attention to. and removed focus: react admin release: highlight Issues that have a high user impact and need to be discussed/paid attention to. labels Sep 19, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Sep 19, 2022 •
edited
Loading

Test Results Summary

Commit SHA: 81c91fb

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests11800201200m 47s
E2E Tests185002018714m 26s
 To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

@vedanshujain vedanshujain requested review from a team and barryhughes and removed request for a team September 19, 2022 16:50
@Konamiman Konamiman self-requested a review September 20, 2022 10:21
@Konamiman Konamiman merged commit c08551c into trunk Sep 20, 2022
@Konamiman Konamiman deleted the fix/34725 branch September 20, 2022 10:22
@github-actions github-actions bot added this to the 7.1.0 milestone Sep 20, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Sep 20, 2022

Hi @Konamiman, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:

  • Add the release: add testing instructions label

roykho pushed a commit that referenced this pull request Sep 20, 2022
@vedanshujain @roykho
Remove typecasting to prevent fatal when $screen_id is null. (#34734)
ca50cc9
@github-actions github-actions bot mentioned this pull request Sep 21, 2022
Closed
roykho pushed a commit that referenced this pull request Sep 21, 2022
@vedanshujain @roykho
Remove typecasting to prevent fatal when $screen_id is null. (#34734)
f798c8f
@seb86
Copy link

seb86 commented Sep 21, 2022

I don't think this is related to just PHP 8.1. This issue occurred for me when running PHP 7.4 too but looking at the error more closely it was caused because I didn't specify a screen ID when using set_current_screen() function in my plugin. I patched it last week and the issue disappeared.

It could be the same for many other plugins that extend the admin like this.

@sybrew
Copy link

sybrew commented Sep 23, 2022

You should remove parameter type hinting/declaration/definition altogether anyway, it makes function calls 6~14% slower; IIRC even per parameter typed. See https://3v4l.org/j7FPnW.

Rely on the PHPdocs you set, and adhere to strict type testing throughout your code altogether (excluding hinting), such as setting the third parameter of in_array() to true, or using === instead of ==.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Konamiman Konamiman Konamiman approved these changes

@barryhughes barryhughes Awaiting requested review from barryhughes

Assignees

No one assigned

Labels

plugin: woocommerce Issues related to the WooCommerce Core plugin.

Projects

None yet

Milestone

7.1.0

Development

Successfully merging this pull request may close these issues.

Potential bug with checking if Screen ID in admin is blank

5 participants

@vedanshujain @seb86 @sybrew @Konamiman