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 Ensure environment is checked before enabling deprecations #11055

Conversation

GuySartorelli
Copy link
Member

@GuySartorelli GuySartorelli commented Nov 19, 2023

michalkleiner
michalkleiner previously approved these changes Nov 19, 2023
Copy link
Contributor

@michalkleiner michalkleiner left a comment

Choose a reason for hiding this comment

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

Good catch, that didn't occur to me when we approved the previous change.

MOG

@GuySartorelli
Copy link
Member Author

I think I've still got it wrong - I'm gonna add checking the static currentlyEnabled to the test. Marking as draft in the meantime.

@GuySartorelli GuySartorelli marked this pull request as draft November 19, 2023 20:59
@michalkleiner michalkleiner dismissed their stale review November 19, 2023 21:11

PR turned to draft for further changes

@GuySartorelli GuySartorelli force-pushed the pulls/4.13/fix-deprecation-checks branch from 0366ccf to 36c6394 Compare November 19, 2023 22:32
@GuySartorelli GuySartorelli marked this pull request as ready for review November 19, 2023 22:32
@GuySartorelli
Copy link
Member Author

There we go, this should be correct and tests all scenarios.

  • Environment mode is only ever checked if the deprecations are enabled
  • If SS_DEPRECATION_ENABLED is set, we ignore the static $currentlyEnabled

@GuySartorelli GuySartorelli force-pushed the pulls/4.13/fix-deprecation-checks branch from 36c6394 to 24b71f4 Compare November 19, 2023 22:35
@GuySartorelli
Copy link
Member Author

And one last change to make the relationship between the static and the env variable clearer. This is ready for review now.

src/Dev/Deprecation.php Outdated Show resolved Hide resolved
src/Dev/Deprecation.php Outdated Show resolved Hide resolved
@michalkleiner
Copy link
Contributor

It's quite hard to read compared to what it was before, but I think we need both the nested and the elseif stanzas.

  1. if env var set and explicitly disabled -> disabled
  2. if env var set and not disabled -> disable for non-dev, otherwise allow
  3. if env var NOT set, and not enabled via static prop -> disabled
  4. if env var NOT set, and explicitly enabled via static prop -> still disable for non-dev, otherwise allow

@GuySartorelli GuySartorelli force-pushed the pulls/4.13/fix-deprecation-checks branch from 24b71f4 to 52e5b8c Compare November 20, 2023 21:41
Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

This should target 4.13? PR that was originally made targeted 4.13 - #11053

@GuySartorelli GuySartorelli changed the base branch from 5 to 4.13 November 20, 2023 22:03
@GuySartorelli
Copy link
Member Author

Yup, I must have forgotten to swap the target branch. Fixed and force-pushed to kick off tests again.

@GuySartorelli GuySartorelli force-pushed the pulls/4.13/fix-deprecation-checks branch from 52e5b8c to bc6f5f7 Compare November 20, 2023 22:04
@emteknetnz emteknetnz merged commit 7eab49f into silverstripe:4.13 Nov 21, 2023
15 checks passed
@emteknetnz emteknetnz deleted the pulls/4.13/fix-deprecation-checks branch November 21, 2023 20:30
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.

3 participants