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

Allow integer values for a Cookie() name #846

Closed
wants to merge 1 commit into from

Conversation

nosilver4u
Copy link
Contributor

@nosilver4u nosilver4u commented Nov 30, 2023

Fixes #845 so that an exception isn't thrown for legitimate numeric cookie names.

There are two changes:

  1. Modify the check for $name, to allow both string and integer cookie names.
  2. Change the exception thrown to indicate a string OR int is permitted.

Pull Request Type

  • I have checked there is no other PR open for the same change.

This is a:

  • Bug fix

Context

Currently, multiple plugins, and possibly even WP core are throwing errors if someone has a cookie with a numeric name. Would love to see this issue go away, so that folks don't have to resort to deleting cookies in the dev tools of their browser.

Quality assurance

  • This change does NOT contain a breaking change (fix or feature that would cause existing functionality to change).
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added unit tests to accompany this PR.
  • The (new/existing) tests cover this PR 100%.
  • I have (manually) tested this code to the best of my abilities.
  • My code follows the style guidelines of this project.
Fixes WordPress#845 so that an exception isn't thrown for legitimate numeric cookie names.
@jrfnl
Copy link
Member

jrfnl commented Nov 30, 2023

@nosilver4u Aside from the fundamental question of whether this is a valid "fix" (in contrast to the user doing it wrong), a PR will not be reviewed or even considered without unit tests documenting the use case. So without tests, this PR will be closed.

@nosilver4u
Copy link
Contributor Author

I missed an update to the parse() method anyway, so I need to create a new pull request with both changes.
Regarding tests, I had wondered about that, so I'll see what I can figure out on that end.

@nosilver4u nosilver4u closed this Nov 30, 2023
@nosilver4u nosilver4u deleted the patch-1 branch November 30, 2023 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment