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

Remove JP Constants #24

Merged
merged 3 commits into from
May 3, 2019
Merged

Remove JP Constants #24

merged 3 commits into from
May 3, 2019

Conversation

kraftbj
Copy link
Contributor

@kraftbj kraftbj commented May 2, 2019

Added to the beta plugin in Automattic/jetpack-beta#72 (when/if merged).

I could see the rationale in keeping it here for times Jetpack is installed without the beta plugin, but can reset the PR to only return early if so desired.

@kraftbj kraftbj requested a review from oskosk May 2, 2019 15:35
@oskosk
Copy link
Collaborator

oskosk commented May 2, 2019

This page could still be useful for sandboxing APIs even with latest stable Jetpack,

@kraftbj
Copy link
Contributor Author

kraftbj commented May 2, 2019

Yeah, I wasn't sure if we would want to say "just install the beta plugin, but still run stable, to mess with constants" or keep the file with conditionals in one/both places.

Up to you boss.

@oskosk
Copy link
Collaborator

oskosk commented May 2, 2019

I'd just remove the beta blocks piece and leave the page here. I think I've only used sandboxing with stable Jetpack so far.

@kraftbj
Copy link
Contributor Author

kraftbj commented May 2, 2019

I've sandboxed with beta versions, like testing out something like Automattic/jetpack#9802 before the wpcom side was deployed.

I've switched it out so that JN's version won't display if the beta plugin is active. As I left it, there's a corner case where if you've set constants with the beta plugin disabled, then enable the beta plugin, you're stuck unable to change constants (since we have the tamper function load without a hook and companion will load first). If this route is an acceptable compromise to you, I can explore a way to juggle them better.

@oskosk
Copy link
Collaborator

oskosk commented May 3, 2019

Awesomeness. Merging

Copy link
Collaborator

@oskosk oskosk left a comment

Choose a reason for hiding this comment

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

LGTM

@oskosk oskosk merged commit 4353e47 into master May 3, 2019
@oskosk oskosk deleted the remove/constants branch May 3, 2019 11:22
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.

2 participants