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

Update modLexicon.php #15377

Merged
merged 1 commit into from Mar 2, 2021
Merged

Update modLexicon.php #15377

merged 1 commit into from Mar 2, 2021

Conversation

BobRay
Copy link
Contributor

@BobRay BobRay commented Feb 6, 2021

Sanity check to prevent PHP error if $_SESSION is not set

What does it do?

Make sure $_SESSION variable exists before it's referenced

Why is it needed?

Existing code breaks automated acceptance tests (and possibly unit tests) where $_SESSION isn't necessarily set.

Related issue(s)/PR(s)

Issue: #15373

@cla-bot
Copy link

cla-bot bot commented Feb 6, 2021

Thank you for your contribution! Before your pull request can be reviewed, please sign the MODX Contributor License Agreement (CLA). This is to make sure MODX has your written permission to distribute projects containing your contributions under the appropriate license.

Please make sure the CLA has been signed for GitHub user(s): @BobRay

Once you've signed, please reply with @cla-bot check to verify and update the status of this pull request.

@BobRay
Copy link
Contributor Author

BobRay commented Feb 6, 2021 via email

@cla-bot cla-bot bot added the cla-signed CLA confirmed for contributors to this PR. label Feb 6, 2021
@cla-bot
Copy link

cla-bot bot commented Feb 6, 2021

The cla-bot has been summoned, and re-checked this pull request!

@Ibochkarev Ibochkarev added the pr/review-needed Pull request requires review and testing. label Feb 6, 2021
@Ibochkarev Ibochkarev added this to the v3.0.0-alpha3 milestone Feb 7, 2021
core/src/Revolution/modLexicon.php Outdated Show resolved Hide resolved
core/src/Revolution/modLexicon.php Outdated Show resolved Hide resolved
@BobRay
Copy link
Contributor Author

BobRay commented Feb 20, 2021

Using the null coalescing operator is fine with me.

@opengeek
Copy link
Member

opengeek commented Mar 2, 2021

I modified this to use the null coalescing operator rather than setting the global variable $_SESSION. Though chances are slim this would ever have a real-world consequence, setting the value of a PHP superglobal to avoid an error seemed like the wrong approach.

@opengeek opengeek dismissed alroniks’s stale review March 2, 2021 21:16

Accepted your null coalescing operator suggestion.

@opengeek opengeek merged commit df3edb0 into modxcms:3.x Mar 2, 2021
@BobRay BobRay deleted the patch-6 branch March 3, 2021 16:29
@BobRay
Copy link
Contributor Author

BobRay commented Mar 3, 2021

I modified this to use the null coalescing operator rather than setting the global variable $_SESSION. Though chances are slim this would ever have a real-world consequence, setting the value of a PHP superglobal to avoid an error seemed like the wrong approach.

It regularly crashed Codeception acceptance tests of the MODX Manager, so I'm glad it's fixed. Using the null coalescing operator is a much better approach than mine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed CLA confirmed for contributors to this PR. pr/review-needed Pull request requires review and testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants