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 context to action processing #329

Merged
merged 1 commit into from
Jul 30, 2019
Merged

Add context to action processing #329

merged 1 commit into from
Jul 30, 2019

Conversation

thenbrent
Copy link
Contributor

@thenbrent thenbrent commented Jul 12, 2019

Add context to action processing / queue runners so that we can log where the action are run from, e.g. WP CLI, WP Cron the Admin List Table or the upcoming Async Runner.

Very handy for debugging issues after the fact.

Example logs entries: https://cld.wthms.co/fT0T96

Fixes #129

@thenbrent thenbrent added this to the 3.0.0 milestone Jul 12, 2019
@thenbrent thenbrent force-pushed the issue_129 branch 9 times, most recently from 6bdbeb1 to 70bbe09 Compare July 15, 2019 05:04
Copy link
Contributor

@rrennick rrennick left a comment

Choose a reason for hiding this comment

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

If we did the context as a slug => Name then the slug could be used by hooks and the Name would be translatable for messages/output. The context array could be stored so only the slug needed to be passed.

@thenbrent
Copy link
Contributor Author

I looked at making the context available for translation, but it's a proper noun in most cases - WP CLI, WP Cron - and pretty close to a proper noun in others - Admin List Table and Async Request - so I decided to instead leave it out of translation.

Are there any benefits to using slugs elsewhere? e.g. wp_cron instead of 'WP Cron' (other than avoiding strings in places you might not expect whitespace).

The context array could be stored so only the slug needed to be passed.

We'd also need to make sure it's got a filter.

@rrennick
Copy link
Contributor

I looked at making the context available for translation, but it's a proper noun in most cases

That sounds good.

Some of the unit tests need to be updated for message string changes.

@thenbrent
Copy link
Contributor Author

Some of the unit tests need to be updated for message string changes.

@rrennick fixed. 👍

Copy link
Contributor

@rrennick rrennick left a comment

Choose a reason for hiding this comment

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

@thenbrent LGTM. There is a merge conflict on hook parameter numbers. Once those are fixed this should be okay to merge.

So that we can log where the action was run from, e.g. WP CLI, WP Cron
the Admin List Table or the upcoming Async Runner.

Very handy for debugging issues after the fact.

Fixes #129
@thenbrent
Copy link
Contributor Author

Thanks @rrennick. I've rebased in a fix for the merge conflict.

@rrennick rrennick merged commit 48d5492 into version_3_0_0 Jul 30, 2019
@delete-merged-branch delete-merged-branch bot deleted the issue_129 branch July 30, 2019 17:40
rrennick added a commit that referenced this pull request Mar 5, 2020
rrennick added a commit that referenced this pull request Mar 13, 2020
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.

2 participants