Place Ad Blocking Recovery Tags on the website #7212
Conversation
|
Build files for 136eb77 have been deleted. |
There was a problem hiding this comment.
Thanks @kuasha420 – this is looking great, just a few things to address.
| class WP_Query_404_Guard implements Guard_Interface { | ||
| /** | ||
| * Determines whether the guarded tag can be activated or not. | ||
| * | ||
| * @since n.e.x.t | ||
| * | ||
| * @return bool TRUE if guarded tag can be activated, otherwise FALSE or an error. | ||
| */ | ||
| public function can_activate() { | ||
| return ! is_404(); |
There was a problem hiding this comment.
This works but we can make it easier to test by providing a WP_Query to this via the constructor. The global function is a wrapper for calling this on WP_Query anyways so it's almost the same thing but using dependency injection.
There was a problem hiding this comment.
Do we have access to the WP_Query object inside the AdSense module without using global? If not and we need to use global there, then I think the current solution makes more sense and looks cleaner. Also, the current tests are pretty simple as well, we don't need to specifically make it easier.
What do you think?
There was a problem hiding this comment.
Do we have access to the
WP_Queryobject inside theAdSensemodule without usingglobal
Nope! That's ok though. The module classes are already very interconnected with many other parts, so it wouldn't be a big change there.
I don't feel super strongly about it, and I agree that it's simple in this form too, my thought is that it's better to make the class work as a pure function when possible.
| 2 | ||
| ); | ||
|
|
||
| $this->do_init_tag_action(); |
There was a problem hiding this comment.
I'm not sure we should do this here because this will fire the same action as the adsense tag.
There was a problem hiding this comment.
Good point, looks like we can't use the get_tag_blocked_on_consent_attribute and other goodies that Module_Web_Tag provides as well for ABR. Should we just extend from Blockable_Tag_Interface or override the do_init_tag_action here?
There was a problem hiding this comment.
Yeah good point about the blocked on consent stuff as well. This looks good (implementing as a basic Tag instead of a Module_Tag) as it will "inherit" the blocking from the main tag, if blocked. There's nothing really to need consent for either since these tags don't track the user or serve ads, etc.
tests/phpunit/integration/Core/Tags/Guards/WP_Query_404_GuardTest.php
Outdated
Show resolved
Hide resolved
| * | ||
| * @since n.e.x.t | ||
| * @access private | ||
| * @ignore | ||
| */ | ||
| class Ad_Blocking_Recovery_Web_Tag extends Module_Web_Tag { | ||
| class Ad_Blocking_Recovery_Web_Tag extends Tag { |
| * @dataProvider data_can_not_activate | ||
| */ | ||
| public function test_can_not_activate( $settings ) { | ||
| $this->settings->merge( $settings ); | ||
| $this->assertFalse( $this->guard->can_activate() ); | ||
| } | ||
|
|
||
| public function test_cant_activate_when_useAdBlockerDetectionSnippet_is_falsy() { | ||
| $this->settings->merge( array( 'useAdBlockerDetectionSnippet' => false ) ); | ||
| $this->assertFalse( $this->guard->can_activate(), 'Should return FALSE when useAdBlockerDetectionSnippet is falsy.' ); | ||
| public function data_can_not_activate() { |
| 2 | ||
| ); | ||
|
|
||
| $this->do_init_tag_action(); |
There was a problem hiding this comment.
Yeah good point about the blocked on consent stuff as well. This looks good (implementing as a basic Tag instead of a Module_Tag) as it will "inherit" the blocking from the main tag, if blocked. There's nothing really to need consent for either since these tags don't track the user or serve ads, etc.
Summary
Addresses issue:
Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist