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

Add `is_transforming_instant_article()` conditional #568

Conversation

Projects
None yet
3 participants
@goldenapples
Copy link
Contributor

goldenapples commented Dec 22, 2016

Adds a new function in the global namespace to check if the current view
being processed is for an instant article. Handles both the RSS feed and
content which is being processed for the Instant_Articles_Publisher
class. This conditional can be checked in any filters on 'the_content',
'the_title' or elsewhere that need to treat Instant Articles content
separately from web content.

See #426

Add `is_instant_article` conditional
Adds a new function in the global namespace to check if the current view
being processed is for an instant article. Handles both the RSS feed and
content which is being processed for the Instant_Articles_Publisher
class. This conditional can be checked in any filters on 'the_content',
'the_title' or elsewhere that need to treat Instant Articles content
separately from web content.

@goldenapples goldenapples force-pushed the goldenapples:426-is-instant-article-conditional branch from c76390a to 8e54394 Dec 22, 2016

Set the status on the existing action hooks
Rather than tracking down every place that might call
Instant_Articles_Post::to_instant_article and wrapping it call to set
the is_instant_article conditional, it's much simpler to just hook those
settings on the existing actions `instant_articles_before_transform_post`
and `instant_articles_after_transform_post`.

@goldenapples goldenapples force-pushed the goldenapples:426-is-instant-article-conditional branch from 6a3dce7 to f81998e Dec 22, 2016

@goldenapples

This comment has been minimized.

Copy link
Contributor Author

goldenapples commented Dec 22, 2016

I know that this logic could easily be done in a theme or another plugin that interfaces with this, now that the actions before and after transforming post are available, but since this has been a common need of mine, it would be nice to have something like this available directly in this plugin itself.

This would also make issues like #252 and #457 a lot simpler to do...

@goldenapples goldenapples changed the title Add `is_instant_article` conditional Add `is_instant_article()` conditional Dec 22, 2016

@diegoquinteiro
Copy link
Collaborator

diegoquinteiro left a comment

Love the simplicity of this solution. It'll be very useful indeed! I just think the method name needs to tell you it'll be true during the transformation phase more explicitly.

* @param bool Set the status
* @return bool
*/
function is_instant_article( $set_status = null ) {

This comment has been minimized.

Copy link
@diegoquinteiro

diegoquinteiro Jan 24, 2017

Collaborator

what about is_transforming_instant_article?

This comment has been minimized.

Copy link
@goldenapples

goldenapples Jan 24, 2017

Author Contributor

Good sugeestion. I've updated the method name in aa33ecf.

@rinatkhaziev

This comment has been minimized.

Copy link
Contributor

rinatkhaziev commented Jan 24, 2017

@goldenapples in f81998e you attach is_instant_article to before.. and after..., this is potentially a point of breakage since hooks can be manipulated outside the context of the plugin. I feel like it's safer to put calls inside Instant_Articles_Post::to_instant_article directly.

cc @diegoquinteiro

goldenapples added some commits Jan 24, 2017

Rename function for clarity
Rename `is_instant_article` to `is_transforming_instant_article`, to
better indicate that it will return true when a tranformation is being
processed.
Don't depend on the before_/after_ hooks
Since these hooks can be modified in plugins, it's safer to set the
status of is_transforming_instant_article() directly in the
`Instant_Articles_Post::to_instant_article` method.
@goldenapples

This comment has been minimized.

Copy link
Contributor Author

goldenapples commented Jan 24, 2017

@rinatkhaziev Good call. I was trying to port over the ad hoc solution I was using in a theme to the plugin. But if this method is going into the plugin directly, it does make much more sense to set the status directly rather than plugging in to those hooks. Does ca03891 look like what you had in mind?

@goldenapples goldenapples changed the title Add `is_instant_article()` conditional Add `is_transforming_instant_article()` conditional Jan 24, 2017

@rinatkhaziev

This comment has been minimized.

Copy link
Contributor

rinatkhaziev commented Jan 24, 2017

@diegoquinteiro This looks good to me, should we merge it into master and then I'm going to fix my PR?

@diegoquinteiro
Copy link
Collaborator

diegoquinteiro left a comment

:shipit:

@diegoquinteiro diegoquinteiro merged commit 41e828f into Automattic:master Jan 25, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@diegoquinteiro

This comment has been minimized.

Copy link
Collaborator

diegoquinteiro commented Jan 25, 2017

@rinatkhaziev done! =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.