Conversation
|
@mike-douglas Sorry about the merge conflicts. It appears by adding in my suggestions to PR#39 caused the conflict. But it should be very quick to review, if you are willing. |
…ying to use them!
mike-douglas
left a comment
mike-douglas
left a comment
There was a problem hiding this comment.
Hi Mike! Just added a few comments, which are related to resolving the conflicts between the changes made from #39 that are now in master. If you could refactor this work based on that I would be happy to merge!
Thanks!
|
|
||
| } | ||
|
|
||
| function ds_npr_api_query_tags_callback( $i ) { |
There was a problem hiding this comment.
Could you remove this from your patch to prevent the conflict from #39?
| //ds_npr_query_publish_ | ||
| add_settings_field( 'ds_npr_query_publish_' . $i, 'Publish Stories ' . $i, 'nprstory_api_query_publish_callback', 'ds_npr_api_get_multi_settings', 'ds_npr_api_get_multi_settings', $i ); | ||
| register_setting( 'ds_npr_api_get_multi_settings', 'ds_npr_query_publish_' . $i , 'nprstory_validation_callback_select'); | ||
|
|
There was a problem hiding this comment.
Could you remove this from your patch to prevent the conflict from #39?
| add_settings_section( 'ds_npr_api_get_multi_settings', 'NPR API multiple get settings', 'nprstory_api_get_multi_settings_callback', 'ds_npr_api_get_multi_settings' ); | ||
|
|
||
| add_settings_field( 'ds_npr_num', 'Number of things to get', 'nprstory_api_num_multi_callback', 'ds_npr_api_get_multi_settings', 'ds_npr_api_get_multi_settings' ); | ||
|
|
There was a problem hiding this comment.
Could you remove this from your patch to prevent the conflict from #39?
| $pub_flag = TRUE; | ||
| } | ||
| $story = $api->update_posts_from_stories($pub_flag); | ||
| $story = $api->update_posts_from_stories($pub_flag, "query_number={$i}" ); |
There was a problem hiding this comment.
The second argument for update_posts_from_stories just takes an integer
classes/NPRAPIWordpress.php
Outdated
| * } | ||
| * @return int|null $post_id or null | ||
| */ | ||
| function update_posts_from_stories( $publish = TRUE ) { |
There was a problem hiding this comment.
If you could, resubmit your PR with the most recent master branch, which has the $qnum parameter added to this function instead of an $opts array or WP arg string
There was a problem hiding this comment.
I will be happy to remove my last changeset if that's what you would prefer.
But before I do may I ask to make sure you were aware they were based on the suggestions I made to PR #39? I added comments inline to the changeset but I don't see the comments I added: https://github.com/nprds/nprapi-wordpress/pull/39/files
I basically suggested that you future proof and not add a numeric 2nd parameter to update_posts_from_stories() but instead add an $opts array that would contain the query_number so that in future if you need to add other parameters you would have end up with a list of many ordinal parameters.
But of course, feel free to decline this modification if you feel it is unimportant in which case I will revert that last commit from my PR.
2 participants
We are running into a problem where an extra large image (
height="4727"andwidth="6306") is causing the Nginx server to just bail with an502 Bad GatewayinWP_Image_Editor_Imagick->thumbnail_image()on the$this->image->resizeImage( $dst_w, $dst_h, $filter, 1 )line when$dst_wand$dst_hare large. Not always, but sometimes.So I've added an
'npr_image_crop_url'hook to allow a site to catch extra large images and perform alternate processing on them (such as having them resized using rsz.io, like this: 1200px wide image.I hope you can review this pull request soon and also that you will accept it, to allow Atlanta's WABE.org to continue using the "official" plugin.