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: avoid deprecation notices in php 8.1 when passing null to strlen #253

Merged

Conversation

drieschel
Copy link
Contributor

The following deprecation notice is popping up in conjunction with php 8.1 when calling $optimizelyClient->setForcedVariation(...); and the experiment is not active for the related environment.

PHP Deprecated: strlen(): Passing null to parameter #1 ($string) of type string is deprecated in /path/to/project/vendor/optimizely/optimizely-sdk/src/Optimizely/DecisionService/DecisionService.php on line 557

In order to fix this it is necessary to cast $experimentId and $variationId to string when passing them to strlen(...) function in Optimizely\DecisionService\DecisionService::setForcedVariation method.

Detailed infos about the deprecation notice can be found here: https://php.watch/versions/8.1/internal-func-non-nullable-null-deprecation

@drieschel drieschel force-pushed the fix/avoid_strlen_deprecated_notices branch from 8b7d003 to 144813d Compare August 29, 2022 20:08
@drieschel
Copy link
Contributor Author

Hello @mikechu-optimizely,

I saw you reviewed another PR. Can you also check this one or add the right reviewers? In case something is missing for a proper PR please let me know, so I will be able to fix that.

@mikechu-optimizely
Copy link
Contributor

Hi @drieschel,

Thanks for giving us your time and expertise.

I've forwarded this into our queue as well as notified our SDK Manager and PHP dev (you don't want me reviewing 😁).

Apologies for the delayed response, but look for me to reply within the next week or feel free to (a)mention me as well.

Back shortly,
Mike

@mikechu-optimizely
Copy link
Contributor

Hi @drieschel,

I'm bringing up your PR in our stand-up meeting today.

Expect to hear from me today or tomorrow with at least an update.

Thanks for your patience.

--Mike

@mikechu-optimizely
Copy link
Contributor

Hi Again @drieschel,

We've added this Pull Request into our next sprint for review.

While we're moving the task forward, please review the Contributing.md. We'll need you to sign the Contributor License Agreement (CLA) linked in that file.

Thanks,
Mike

@drieschel
Copy link
Contributor Author

Thank you @mikechu-optimizely! I signed the CLA. Please let me know if you need anything else from my side.

@mikechu-optimizely
Copy link
Contributor

I see your information and agreement. Danke.

Please look towards an update within the next 2 weeks (hopefully less) during our next sprint.

@mikechu-optimizely
Copy link
Contributor

Well, I guess you are stuck with me. The task item showed up on my board for this next sprint. Time to go brush off my old PHP skills.

My tasking is to research the effect of this change, along with running tests on your solution (direct cast). Thanks for providing the resource/research in your original submission.

Back w/ you this week on this....

@drieschel
Copy link
Contributor Author

Thanks for keeping me updated @mikechu-optimizely and sorry for the inconveniences.

@mikechu-optimizely
Copy link
Contributor

No inconvenience at all. Thanks for taking the time to contribute.

@coveralls
Copy link

coveralls commented Sep 30, 2022

Coverage Status

Coverage increased (+0.02%) to 97.149% when pulling e13628f on drieschel:fix/avoid_strlen_deprecated_notices into 60de6df on optimizely:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 97.292% when pulling e13628f on drieschel:fix/avoid_strlen_deprecated_notices into 60de6df on optimizely:master.

Copy link
Contributor

@msohailhussain msohailhussain left a comment

Choose a reason for hiding this comment

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

lgtm
let me check with our integration tests and we are good to go.

@msohailhussain msohailhussain merged commit fa9a3bf into optimizely:master Sep 30, 2022
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.

4 participants