Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#34455 closed defect (bug) (fixed)

Additional Filters for Press This

Reported by: cadeyrn's profile cadeyrn Owned by: kraftbj's profile kraftbj
Milestone: 4.5 Priority: high
Severity: normal Version: 4.2
Component: Press This Keywords: has-patch
Focuses: Cc:

Description

I've added 3 filters for the Press This class:

  1. 'press_this_save_post_content'

Applied right after the side_load_images in order to allow potential side loads of other types of media.
Example use case: side load non-image media, such as audio or video.

  1. 'press_this_useful_html_elements'

Allows filtering of currently hardcoded array of HTML elements allowed in fetch_source_html step for special cases where additional HTML elements need to be kept.
Example use case: HTML5 elements, such as amp-img, that someone wants to pull in.

  1. 'press_this_suggested_content'

A filter for the content right before it's passed to the editor and presented to the user.
Example use case is when someone stored posts in a different, non-HTML format, such as Markdown, this is essential.

Attachments (3)

class-wp-press-this.php.patch (1.4 KB) - added by cadeyrn 9 years ago.
patch for class-wp-press-this.php for additional filters
34455.diff (1.7 KB) - added by kraftbj 9 years ago.
Minor coding standard update of the OP and fixed a minor typo.
34455.1.patch (2.0 KB) - added by azaozz 9 years ago.

Download all attachments as: .zip

Change History (29)

@cadeyrn
9 years ago

patch for class-wp-press-this.php for additional filters

#1 @cadeyrn
9 years ago

  • Keywords has-patch added

#2 follow-up: @kraftbj
9 years ago

  • Keywords 2nd-opinion added
  • Version changed from 4.3.1 to 4.2

Howdy Cadeyrn! My apologies for not replying to this earlier and thank you for the patch.

As a best practice, it is best to create a patch from the checkout root so the full path is there. The filters do sound useful.

@kraftbj
9 years ago

Minor coding standard update of the OP and fixed a minor typo.

#3 @kraftbj
9 years ago

  • Milestone changed from Awaiting Review to 4.5
  • Owner set to kraftbj
  • Status changed from new to accepted

#4 in reply to: ↑ 2 @cadeyrn
9 years ago

Thank you for the cleanup and the help; next time I'll include the full path, thank you.

#5 @SergeyBiryukov
9 years ago

  • Keywords commit added

#6 @ocean90
9 years ago

  • Keywords 2nd-opinion removed
  • Type changed from feature request to enhancement

This ticket was mentioned in Slack in #core by jorbin. View the logs.


9 years ago

#8 @jorbin
9 years ago

  • Milestone changed from 4.5 to Future Release

No real activity in multiple weeks, so this is going to miss 4.5

#9 @kraftbj
9 years ago

@jorbin What is lacking? The patch is good, as far as I know, and it was marked commit.

#10 @jorbin
9 years ago

  • Milestone changed from Future Release to 4.5

@kraftbj it was the lack of activity and the fact that it's been 7 weeks with the commit tag and no commit.

It is lacking the version in the since, but that is minor. You showed interest and it's before the deadline, so I'll finish up the docs and commit it.

#11 @jorbin
9 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 36672:

Add Additional filters to Press This

3 new filters that aim to improve the extensibility of Press This:
1) press_this_save_post_content - Applied right after the side_load_images in order to allow potential side loads of other types of media.
Example use case: side load non-image media, such as audio or video.

2) press_this_useful_html_elements
Allows filtering of currently hard coded array of HTML elements allowed in fetch_source_html step for special cases where additional HTML elements need to be kept.
Example use case: HTML5 elements, such as amp-img, that someone wants to pull in.

3) press_this_suggested_content
A filter for the content right before it's passed to the editor and presented to the user.
Example use case is when someone stored posts in a different, non-HTML format, such as Markdown, this is essential.

Fixes #34455.
Props cadeyrn, kraftbj

#12 @DrewAPicture
9 years ago

@jorbin I think we should probably rename the press_this_useful_html_elements hook to press_this_allowed_tags. It is, in and of itself a list of allowed tags. I don't know that they're so much "useful" as "allowed".

#13 follow-up: @azaozz
9 years ago

I'm not sure that any of these filters is particularly useful.

  • press_this_save_post_content is yet another filter that runs when a post is saved. There are already a few other filters that can be used to do exactly the same.
  • press_this_useful_html_elements runs only when the content _is not_ passed by the bookmarklet, i.e. most of the time it cannot be used. As far as I see it can be useful only when repurposing Press This for other user cases. However that is not supported, and not sure if we should encourage it.
  • press_this_suggested_content is very similar to press_this_suggested_html (even the name sounds confusing when comparing them). It filters the return of press_this_suggested_html after it has been wrapped in HTML tags. Again, only useful if repurposing Press This to do something that was not intended.

If plugins want to repurpose Press This, they are free to do so. However this is not the intended use and will probably break when Press This is updated.

#14 in reply to: ↑ 13 ; follow-up: @cadeyrn
9 years ago

@azaozz

I'm not sure that any of these filters is particularly useful.

  • press_this_save_post_content is yet another filter that runs when a post is saved. There are already a few other filters that can be used to do exactly the same.

Is sideloading from the referenced url (which lead to Press This) possible on regular post save?

  • press_this_useful_html_elements runs only when the content _is not_ passed by the bookmarklet, i.e. most of the time it cannot be used. As far as I see it can be useful only when repurposing Press This for other user cases. However that is not supported, and not sure if we should encourage it.

Why would it be repurposing just because some might find other HTML elements useful as well?

  • press_this_suggested_content is very similar to press_this_suggested_html (even the name sounds confusing when comparing them). It filters the return of press_this_suggested_html after it has been wrapped in HTML tags. Again, only useful if repurposing Press This to do something that was not intended.

This is not true. I'm using Markdown when writing posts and without this, I can't do so in Press this.

#15 in reply to: ↑ 14 @azaozz
9 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

@cadeyrn

Is sideloading from the referenced url (which lead to Press This) possible on regular post save?

I may be missing something but "sideloading" is possible at any time, when saving a post or not. You have a choice of media_sideload_image(), media_handle_sideload() or wp_handle_sideload() depending on how much control you want to have.

Why would it be repurposing just because some might find other HTML elements useful as well?

What I mean is that the press_this_useful_html_elements filter only runs when not using the bookmarklet and an URL is typed in PT directly. This is a compatibility mode that's rarely used. So this filter will never run in normal, intended use of PT. In addition it is somewhat confusing as plugin authors may expect it to always run.

This is not true. I'm using Markdown when writing posts and without this, I can't do so in Press this.

The press_this_suggested_content filter has nothing to do with using Markdown (or anything else) in the editor. If you don't want the default HTML tags, you can remove them completely or replace with whatever you need by using press_this_suggested_html. I don't see why we need another filter for almost the same thing. In addition press_this_suggested_content is harder to use as the only way would be to parse the HTML with regexp, and that is... not good :)

Reopening for reconsideration. At least the names need to be better.

#16 @ocean90
9 years ago

  • Keywords needs-patch added; has-patch commit removed

#17 @DrewAPicture
9 years ago

In 36819:

Docs: Match the parameter name in the hook docs for the press_this_useful_html_elements filter, introduced in [36672].

See #34455. See #35986.

@azaozz
9 years ago

#18 follow-ups: @azaozz
9 years ago

  • Keywords has-patch added; needs-patch removed
  • Priority changed from normal to high
  • Type changed from enhancement to defect (bug)

In 34455.1.patch:

  • Rename press_this_save_post_content to press_this_save_post and pass all post data to it. This is still marginally useful as there are at least two other "save post" filters that can be used, however if we must have it at least lets filter all of the post data.
  • Remove press_this_useful_html_elements. It's misleading as it runs only in compatibility mode. Also we are looking at keeping some of the HTML elements in the bookmarklet, then we will have to sync this or remove all, depending on the direction that goes.
  • Remove press_this_suggested_content. It is completely useless as all of the suggested content can be controlled better by using press_this_suggested_html.

#19 in reply to: ↑ 18 @cadeyrn
9 years ago

Replying to azaozz:

  • Remove press_this_suggested_content. It is completely useless as all of the suggested content can be controlled better by using press_this_suggested_html.

No, it can't. press_this_suggested_html runs before the content is extended with additional strings.
https://core.trac.wordpress.org/browser/tags/4.4.2/src/wp-admin/includes/class-wp-press-this.php#L1171

Version 0, edited 9 years ago by cadeyrn (next)

#20 in reply to: ↑ 18 ; follow-up: @cadeyrn
9 years ago

Replying to azaozz:

  • Remove press_this_suggested_content. It is completely useless as all of the suggested content can be controlled better by using press_this_suggested_html.

I can't find any filter for the $text variable in get_suggested_content. It is not part of the $default_html that is filtered with press_this_suggested_html.

If the press_this_suggested_content is getting dropped, could that $text be included in the filtered array?

#21 in reply to: ↑ 20 ; follow-up: @azaozz
9 years ago

Replying to cadeyrn:

I can't find any filter for the $text variable in get_suggested_content.

The press_this_suggested_html filter lets you modify (or remove) the HTML tags. Then sprintf() is used to add the content at the specified places in the modified strings. I'm not sure how exactly you're trying to use this, but to get only the $text variable in the content you can do:

add_filter( 'press_this_suggested_html', 'my_press_this_content' );
function my_press_this_content( $array ) {
    return array( 'quote' => '%s' );
}

#22 @kraftbj
9 years ago

Thumbs up to 34455.1. @azaozz's modifications makes sense to me and simplifies the available filters.

#23 in reply to: ↑ 21 ; follow-up: @cadeyrn
9 years ago

Replying to azaozz:

OK, I got it.

The end result can be filtered with press_this_suggested_html; however, the actual content, that is sprint-ed into the 'quote' array element ( $text variable within get_suggested_content ) can be filtered with the press_this_data filter, called in merge_or_fetch_data, so there is no need for a final filter.

Next time I'll spend more time learning the code and adding documentation instead of introducing unneeded filters; thank you for your time, @azaozz.

#24 in reply to: ↑ 23 @kraftbj
9 years ago

Replying to cadeyrn:

Next time I'll spend more time learning the code and adding documentation instead of introducing unneeded filters; thank you for your time.

Don't beat yourself up—I didn't catch there was existing ways to do what you wanted either. Your ticket still added a filter to make Press This more extendable. Thank *you* for your time and for sticking with this issue.

#25 @azaozz
9 years ago

Yeah, it is partially my fault too. Should have added more detailed explanation in the inline docs.

The press_this_suggested_html filter is not the most straightforward way of controlling the suggested content for the editor but gives more granular access that is quite better than parsing the HTML with regex.

#26 @azaozz
9 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 36848:

Pres This:

  • Change the newly added press_this_save_post_content filter to press_this_save_post and pass the $post_data array to it.
  • Remove the newly added press_this_useful_html_elements. It only runs in compatibility mode when a URL is typed by the user.
  • Remove the press_this_suggested_content filter. It is redundant as the suggested HTML for the editor is already filtered by press_this_suggested_html.
  • Add some more inline docs and rename couple of vars to make the code more readable.

Fixes #34455.

Note: See TracTickets for help on using tickets.