-
Notifications
You must be signed in to change notification settings - Fork 2
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
disable Apple News pushes on non-production environments #44
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few questions and suggestions. :)
src/alley/wp/alleyvate/features/class-disable-apple-news-no-prod-push.php
Outdated
Show resolved
Hide resolved
src/alley/wp/alleyvate/features/class-disable-apple-news-no-prod-push.php
Outdated
Show resolved
Hide resolved
src/alley/wp/alleyvate/features/class-disable-apple-news-no-prod-push.php
Outdated
Show resolved
Hide resolved
src/alley/wp/alleyvate/features/class-disable-apple-news-no-prod-push.php
Outdated
Show resolved
Hide resolved
src/alley/wp/alleyvate/features/class-disable-apple-news-no-prod-push.php
Outdated
Show resolved
Hide resolved
tests/alley/wp/alleyvate/features/test-disable-apple-news-no-prod-push.php
Outdated
Show resolved
Hide resolved
Co-authored-by: Renato Alves <[email protected]>
…s' into feature/disable-apple-news-pushes
src/alley/wp/alleyvate/features/class-disable-apple-news-non-prod-push.php
Show resolved
Hide resolved
src/alley/wp/alleyvate/features/class-disable-apple-news-non-prod-push.php
Outdated
Show resolved
Hide resolved
src/alley/wp/alleyvate/features/class-disable-apple-news-non-prod-push.php
Outdated
Show resolved
Hide resolved
src/alley/wp/alleyvate/features/class-disable-apple-news-non-prod-push.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Temporary review
I'd recommend also disabling remote images when not on production. Otherwise, Apple News will trigger an error and fail the push if it tries to fetch remote images that it doesn't have access to, i.e. from local envs or protected non-production envs. |
this is disabling all pushes, so the image references won't matter. |
@mogmarsh Ah, right. I was thinking of scenarios in which we have test credentials on non-prod. In that case, I'd suggest also disabling if |
@emilyatmobtown's input is valid, but should come in the form of a future issue and not block this one. I see preventing prod bulk CLI updates as a separate feature from blocking pushes on lower environments. |
https://github.com/alleyinteractive/wp-bulk-task/blob/main/src/trait-bulk-task-side-effects.php also could handle this in many CLI situations if we use it. |
Extensive revisions. New reviews required.
* the method caches the initial result and always returns it, even if | ||
* we modify the environment variable. | ||
*/ | ||
\define( 'WP_RUN_CORE_TESTS', true ); // phpcs:ignore WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedConstantFound |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fwiw, mantle will set this as of v1.0.4: https://github.com/alleyinteractive/mantle-framework/blob/4961ae56da6464d0b4732f075ea8e9ca2b3e2d89/src/mantle/testing/wordpress-bootstrap.php#L106-L108
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. It wasn't, which is why I had to add it here. Maybe worth a look?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohhh I missed the asof
line. That's probably why.
Summary
As titled.
Notes for reviewers
None.
Changelog entries
Added
Changed
Deprecated
Removed
Fixed
Security