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

Posts Connector: Enable on the front end to ensure logging happens for REST API events #1264

Merged
merged 1 commit into from
Oct 18, 2021

Conversation

coreymckrill
Copy link
Contributor

Post creation and updating that happens through the REST API is not considered to occur in the "admin" even if the request is made from the block editor. Therefore, these events aren't getting logged because the Posts connector is disabled for the frontend. I don't know what the rationale is for disabling it, but by not doing that, post creation gets logged.

Fixes #1195
Fixes #1250

Checklist

  • Project documentation has been updated to reflect the changes in this pull request, if applicable.
  • I have tested the changes in the local development environment (see contributing.md).
  • I have added phpunit tests.

Post creation and updating that happens through the REST API is not considered to occur in the "admin" even if the request is made from the block editor. Therefore, these events aren't getting logged because the Posts connector is disabled for the frontend. I don't know what the rationale is for disabling it, but by _not_ doing that, post creation gets logged.

Fixes #1250
@kasparsd
Copy link
Contributor

Thanks for opening a pull request @coreymckrill! Can you please check if this is solving the same problem as #1223?

@coreymckrill
Copy link
Contributor Author

@kasparsd I think that it is. This solution is a lot simpler, but I don't know what the original reasoning was for turning off the Posts connector on the front end.

@coreymckrill
Copy link
Contributor Author

@kasparsd Anything else holding up merging either this or #1223? Would love to have post edits made through rest/block editor get logged

@kidunot89
Copy link
Contributor

kidunot89 commented Jul 7, 2021

@kasparsd I think that it is. This solution is a lot simpler, but I don't know what the original reasoning was for turning off the Posts connector on the front end.

@coreymckrill The original reasoning (which pre-dates Gutenberg's existence) for restricting the Posts Connector from the operating on the Front-end was to avoid recording unnecessary records when wp_update_post() is called if a third-party plugin is designed to do so, however if individual CPTs can be blacklisted, right? 🤔

@kasparsd @coreymckrill Maybe the solution to this issue is adding toggleable option register_frontend for the Posts Connector on the Stream dashboard Settings page.

@coreymckrill
Copy link
Contributor Author

@kidunot89 Ah, thanks for clarifying!

Maybe the solution to this issue is adding toggleable option register_frontend for the Posts Connector on the Stream dashboard Settings page.

Would it work to differentiate between frontend requests and rest api requests?

@coreymckrill
Copy link
Contributor Author

#1269 is an attempt to implement the idea above, differentiating between frontend and api requests.

@dd32
Copy link
Contributor

dd32 commented Oct 18, 2021

@kidunot89 @kasparsd Any chance of moving this forward at all? Stream not capturing post/page edits/creations/etc is a pretty major bug and I'd love not to have to move to a different plugin if I can help it..

@kasparsd
Copy link
Contributor

Sorry for missing all the messages on this issue. Will look at the changeset again.

Copy link
Contributor

@kasparsd kasparsd left a comment

Choose a reason for hiding this comment

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

I don't see any immediate issues with allowing the post connector to run on all requests.

@kasparsd
Copy link
Contributor

kasparsd commented Mar 8, 2022

This changeset somehow made this unit test fail:

https://app.travis-ci.com/github/xwp/stream/jobs/562444098#L1671

1) WP_Stream\Test_WP_Stream_Connector_EDD::test_log_override
Failed asserting that 2 is identical to 4.
/var/www/html/wp-content/plugins/stream-src/tests/tests/connectors/test-class-connector-edd.php:192

public function test_log_override() {
// Callback for validating expected log data.
$asserted = 0;
add_action(
'wp_stream_log_data',
function( $data ) use( &$asserted ) {
if ( 'edd' === $data['connector'] && in_array( $data['context'], array( 'downloads', 'discounts' ), true ) ) {
$asserted++;
}
return $data;
},
99
);
// Create download and discount to trigger logs.
$this->create_simple_download();
$this->create_simple_percent_discount();
// Check assertion flage
$this->assertSame( $asserted, 2 );
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants