Skip to content

Markdown: enable for posts whenever the module is active.#6608

Merged
dereksmart merged 3 commits intomasterfrom
fix/markdown-auto-enable
Mar 9, 2017
Merged

Markdown: enable for posts whenever the module is active.#6608
dereksmart merged 3 commits intomasterfrom
fix/markdown-auto-enable

Conversation

@jeherve
Copy link
Copy Markdown
Member

@jeherve jeherve commented Mar 8, 2017

Enable markdown for posts on plugin upgrade.

The filter that forcefully enabled this when markdown was enabled was removed in #6406. On plugin upgrade it will be re-enabled.

Fixes #6605

Related:

Proposed changelog entry for your changes:

  • Markdown: always enable for posts whenever the module is active.

Also remove the option from Settings > Writing.

Fixes #6605

Related:
- #6406
- #6548
@jeherve jeherve added [Feature] Markdown [Pri] BLOCKER [Status] Needs Review This PR is ready for review. Bug When a feature is broken and / or not performing as intended labels Mar 8, 2017
@jeherve jeherve added this to the 4.7.1 milestone Mar 8, 2017
@jeherve jeherve self-assigned this Mar 8, 2017
@jeherve jeherve requested a review from samhotchkiss March 8, 2017 21:20
@dereksmart dereksmart force-pushed the fix/markdown-auto-enable branch from 3fb4e3d to c54efbd Compare March 8, 2017 22:20
@samhotchkiss
Copy link
Copy Markdown
Contributor

This'll work, but it'll also cause an extra DB hit on every page load. Is there a way to get a little bit more discrete with how/when we call this? Maybe hook it onto plugin_upgrade?

// Step 1. Force the option on whenever this file is loaded (i.e. when the module is acive).
add_filter( 'pre_option_' . WPCom_Markdown::POST_OPTION, '__return_true' );

/**
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@dereksmart Shouldn't we keep hiding that field, to avoid confusion? We don't really want folks to be fiddling with that field, do we?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't see that change in the diff, have you reviewed an older version?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, I had re-added that function in d528053, and it was removed in c54efbd. I think we should keep it.

Copy link
Copy Markdown
Contributor

@dereksmart dereksmart Mar 9, 2017

Choose a reason for hiding this comment

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

@jeherve I agree, and re-removed it

Copy link
Copy Markdown
Contributor

@eliorivero eliorivero left a comment

Choose a reason for hiding this comment

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

This LGTM since it will set the option to true if it doesn't exist. It will also set it to true if it's set to false, but we don't have that situation currently and it will only be done once during plugin upgrade.
We do need to remember to remove this when the new settings UI is released.
cc @tyxla so he's aware of this change. We need this since users activating the module don't have the option set to true (it was previously faked as true with a filter that was always returning true). We're setting it to true only once during next plugin upgrade.

@eliorivero eliorivero added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Mar 9, 2017
@dereksmart
Copy link
Copy Markdown
Contributor

This'll work, but it'll also cause an extra DB hit on every page load. Is there a way to get a little bit more discrete with how/when we call this? Maybe hook it onto plugin_upgrade?

@samhotchkiss, if you look at the conditional this is in, you'll see that it will only hit when the versions do not match, i.e. when the plugin has been upgraded.

Copy link
Copy Markdown
Member Author

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This worked in my tests! LGTM 🚢

@dereksmart dereksmart merged commit 73f17b2 into master Mar 9, 2017
@dereksmart dereksmart deleted the fix/markdown-auto-enable branch March 9, 2017 15:09
@dereksmart dereksmart removed the [Status] Ready to Merge Go ahead, you can push that green button! label Mar 9, 2017
dereksmart pushed a commit that referenced this pull request Mar 9, 2017
* Markdown: enable for posts whenever the module is active.

Also remove the option from Settings > Writing.

Fixes #6605

Related:
- #6406
- #6548

* Markdown: make sure posts markdown is re-enabled on upgrade

* Remove setting checkbox from writing options page
@dereksmart
Copy link
Copy Markdown
Contributor

merged to 4.7 71752a9

jeherve added a commit that referenced this pull request Mar 9, 2017
dereksmart pushed a commit that referenced this pull request Mar 9, 2017
* Update stable tag.

* Move 4.7 changelog to changelog.txt

* Changelog: add #6571

* Changelog: add #6600

* Changelog: add #6604

* Changelog: add #6608

* Changelog: remove PR numbers and add release date and link.

* Changelog: add #6615
dereksmart pushed a commit that referenced this pull request Mar 9, 2017
* Update stable tag.

* Move 4.7 changelog to changelog.txt

* Changelog: add #6571

* Changelog: add #6600

* Changelog: add #6604

* Changelog: add #6608

* Changelog: remove PR numbers and add release date and link.

* Changelog: add #6615
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug When a feature is broken and / or not performing as intended [Feature] Markdown [Pri] BLOCKER

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants