Skip to content

Commit d6a941d

Browse files
GaryJonesclaude
GaryJones
and
claude
committed
fix: prevent empty widget wrapper output when no ad codes found
Resolves issue #72 where the ACM_Ad_Zones widget would output empty wrapper HTML (before_widget/after_widget) even when no valid ad codes were found for the specified tag. This created unnecessary empty markup in the page output. The widget now uses output buffering to capture the ad content first, checks if any content was generated, and only outputs the wrapper HTML if there's actual ad content to display. A new filter 'acm_display_empty_widget' has been added to allow themes to override this behaviour if they need the wrapper HTML to display even when empty (defaults to false). Changes: - Modified ACM_Ad_Zones::widget() to buffer ad output and conditionally render wrapper - Added 'acm_display_empty_widget' filter with ad zone, args, and instance parameters - Excluded short prefix warning from PHPCS for established 'acm' prefix - Added 7 integration tests covering empty output behaviour and filter functionality - Added unit tests verifying get_acm_tag() returns empty string when no ad codes found Co-Authored-By: Claude Opus 4.5 <[email protected]>
1 parent 008f061 commit d6a941d

File tree

4 files changed

+402
-21
lines changed

4 files changed

+402
-21
lines changed

‎.phpcs.xml.dist‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,8 @@
5353
<element value="Automattic\AdCodeManager"/>
5454
</property>
5555
</properties>
56+
<!-- Allow 3-character prefix "acm" - this is an established plugin prefix. -->
57+
<exclude name="WordPress.NamingConventions.PrefixAllGlobals.ShortPrefixPassed"/>
5658
</rule>
5759

5860
<rule ref="WordPress.WP.I18n">

‎src/class-acm-widget.php‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,13 +60,35 @@ function update( $new_instance, $old_instance ) {
6060

6161
// Display the widget
6262
function widget( $args, $instance ) {
63+
// Capture the ad content to check if we have anything to display.
64+
// This fixes issue #72: don't output empty widget wrapper when no ad code found.
65+
ob_start();
66+
do_action( 'acm_tag', $instance['ad_zone'] );
67+
$ad_content = ob_get_clean();
68+
69+
/**
70+
* Filters whether to display the widget when no ad content is found.
71+
*
72+
* @since 0.8.0
73+
*
74+
* @param bool $display Whether to display the widget. Default false.
75+
* @param string $ad_zone The ad zone ID.
76+
* @param array $args Widget display arguments.
77+
* @param array $instance Widget instance settings.
78+
*/
79+
if ( empty( $ad_content ) && ! apply_filters( 'acm_display_empty_widget', false, $instance['ad_zone'], $args, $instance ) ) {
80+
return;
81+
}
82+
6383
echo $args['before_widget'];
6484
$title = apply_filters( 'widget_title', $instance['title'] );
6585

6686
if ( ! empty( $title ) ) {
6787
echo $args['before_title'] . esc_html( $title ) . $args['after_title'];
6888
}
69-
do_action( 'acm_tag', $instance['ad_zone'] );
89+
90+
// phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped -- Ad content is escaped during token replacement in get_acm_tag().
91+
echo $ad_content;
7092
echo $args['after_widget'];
7193
}
7294
}

‎tests/Integration/WidgetTest.php‎

Lines changed: 297 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,297 @@
1+
<?php
2+
/**
3+
* Integration tests for the ACM_Ad_Zones widget.
4+
*
5+
* Tests for issue #72: Ad code tag still rendered if no valid ad codes found.
6+
*
7+
* @package Automattic\AdCodeManager\Tests\Integration
8+
*/
9+
10+
declare( strict_types=1 );
11+
12+
namespace Automattic\AdCodeManager\Tests\Integration;
13+
14+
use ACM_Ad_Zones;
15+
16+
/**
17+
* Test case for ACM_Ad_Zones widget.
18+
*/
19+
class WidgetTest extends TestCase {
20+
21+
/**
22+
* The widget instance.
23+
*
24+
* @var ACM_Ad_Zones
25+
*/
26+
private ACM_Ad_Zones $widget;
27+
28+
/**
29+
* Set up test fixtures.
30+
*/
31+
public function set_up(): void {
32+
parent::set_up();
33+
$this->widget = new ACM_Ad_Zones();
34+
}
35+
36+
/**
37+
* Test widget outputs nothing when no ad code is found.
38+
*
39+
* This is the main fix for issue #72 - the widget should not output
40+
* empty wrapper HTML when there's no ad content to display.
41+
*
42+
* @covers ACM_Ad_Zones::widget
43+
*/
44+
public function test_widget_outputs_nothing_when_no_ad_code_found(): void {
45+
$args = array(
46+
'before_widget' => '<div class="widget">',
47+
'after_widget' => '</div>',
48+
'before_title' => '<h2>',
49+
'after_title' => '</h2>',
50+
);
51+
$instance = array(
52+
'title' => 'Test Ad',
53+
'ad_zone' => 'nonexistent_zone',
54+
);
55+
56+
ob_start();
57+
$this->widget->widget( $args, $instance );
58+
$output = ob_get_clean();
59+
60+
$this->assertEmpty( $output, 'Widget should output nothing when no ad code is found.' );
61+
}
62+
63+
/**
64+
* Test widget outputs nothing for empty ad zone.
65+
*
66+
* @covers ACM_Ad_Zones::widget
67+
*/
68+
public function test_widget_outputs_nothing_for_empty_ad_zone(): void {
69+
$args = array(
70+
'before_widget' => '<div class="widget">',
71+
'after_widget' => '</div>',
72+
'before_title' => '<h2>',
73+
'after_title' => '</h2>',
74+
);
75+
$instance = array(
76+
'title' => '',
77+
'ad_zone' => '',
78+
);
79+
80+
ob_start();
81+
$this->widget->widget( $args, $instance );
82+
$output = ob_get_clean();
83+
84+
$this->assertEmpty( $output, 'Widget should output nothing for empty ad zone.' );
85+
}
86+
87+
/**
88+
* Test widget outputs wrapper when filter returns true for empty content.
89+
*
90+
* The acm_display_empty_widget filter allows themes to force display
91+
* of the widget wrapper even when no ad content is found.
92+
*
93+
* @covers ACM_Ad_Zones::widget
94+
*/
95+
public function test_widget_outputs_wrapper_when_filter_returns_true(): void {
96+
add_filter( 'acm_display_empty_widget', '__return_true' );
97+
98+
$args = array(
99+
'before_widget' => '<div class="widget">',
100+
'after_widget' => '</div>',
101+
'before_title' => '<h2>',
102+
'after_title' => '</h2>',
103+
);
104+
$instance = array(
105+
'title' => '',
106+
'ad_zone' => 'nonexistent_zone',
107+
);
108+
109+
ob_start();
110+
$this->widget->widget( $args, $instance );
111+
$output = ob_get_clean();
112+
113+
remove_filter( 'acm_display_empty_widget', '__return_true' );
114+
115+
$this->assertStringContainsString( '<div class="widget">', $output, 'Widget should output wrapper when filter returns true.' );
116+
$this->assertStringContainsString( '</div>', $output, 'Widget should output closing wrapper when filter returns true.' );
117+
}
118+
119+
/**
120+
* Test widget outputs title when filter allows empty content.
121+
*
122+
* @covers ACM_Ad_Zones::widget
123+
*/
124+
public function test_widget_outputs_title_when_filter_allows_empty_content(): void {
125+
add_filter( 'acm_display_empty_widget', '__return_true' );
126+
127+
$args = array(
128+
'before_widget' => '<div class="widget">',
129+
'after_widget' => '</div>',
130+
'before_title' => '<h2>',
131+
'after_title' => '</h2>',
132+
);
133+
$instance = array(
134+
'title' => 'Advertisement',
135+
'ad_zone' => 'nonexistent_zone',
136+
);
137+
138+
ob_start();
139+
$this->widget->widget( $args, $instance );
140+
$output = ob_get_clean();
141+
142+
remove_filter( 'acm_display_empty_widget', '__return_true' );
143+
144+
$this->assertStringContainsString( '<h2>Advertisement</h2>', $output, 'Widget should output title when filter allows.' );
145+
}
146+
147+
/**
148+
* Test acm_display_empty_widget filter receives correct arguments.
149+
*
150+
* @covers ACM_Ad_Zones::widget
151+
*/
152+
public function test_filter_receives_correct_arguments(): void {
153+
$received_args = array();
154+
155+
$filter_callback = function ( $display, $ad_zone, $args, $instance ) use ( &$received_args ) {
156+
$received_args = array(
157+
'display' => $display,
158+
'ad_zone' => $ad_zone,
159+
'args' => $args,
160+
'instance' => $instance,
161+
);
162+
return false;
163+
};
164+
165+
add_filter( 'acm_display_empty_widget', $filter_callback, 10, 4 );
166+
167+
$args = array(
168+
'before_widget' => '<div class="widget">',
169+
'after_widget' => '</div>',
170+
'before_title' => '<h2>',
171+
'after_title' => '</h2>',
172+
);
173+
$instance = array(
174+
'title' => 'Test',
175+
'ad_zone' => 'test_zone',
176+
);
177+
178+
ob_start();
179+
$this->widget->widget( $args, $instance );
180+
ob_get_clean();
181+
182+
remove_filter( 'acm_display_empty_widget', $filter_callback, 10 );
183+
184+
$this->assertFalse( $received_args['display'], 'First argument should be false.' );
185+
$this->assertSame( 'test_zone', $received_args['ad_zone'], 'Second argument should be the ad zone.' );
186+
$this->assertSame( $args, $received_args['args'], 'Third argument should be widget args.' );
187+
$this->assertSame( $instance, $received_args['instance'], 'Fourth argument should be widget instance.' );
188+
}
189+
190+
/**
191+
* Test widget with valid ad code outputs content.
192+
*
193+
* @covers ACM_Ad_Zones::widget
194+
*/
195+
public function test_widget_outputs_content_with_valid_ad_code(): void {
196+
// Create a valid ad code.
197+
$ad_code_data = array();
198+
foreach ( $this->acm->current_provider->ad_code_args as $arg ) {
199+
$ad_code_data[ $arg['key'] ] = 'test_value_' . $arg['key'];
200+
}
201+
$ad_code_data['priority'] = 10;
202+
$ad_code_data['operator'] = 'AND';
203+
204+
// Need to set up a tag that's registered.
205+
// First, let's check what tags are available.
206+
$ad_tag_ids = $this->acm->ad_tag_ids;
207+
if ( empty( $ad_tag_ids ) ) {
208+
$this->markTestSkipped( 'No ad tag IDs available for testing.' );
209+
}
210+
211+
// Get the first tag with enable_ui_mapping.
212+
$test_tag = null;
213+
foreach ( $ad_tag_ids as $tag ) {
214+
if ( isset( $tag['enable_ui_mapping'] ) && $tag['enable_ui_mapping'] ) {
215+
$test_tag = $tag['tag'];
216+
break;
217+
}
218+
}
219+
220+
if ( ! $test_tag ) {
221+
$this->markTestSkipped( 'No tags with enable_ui_mapping available for testing.' );
222+
}
223+
224+
// Set the tag in ad code data.
225+
$ad_code_data['tag'] = $test_tag;
226+
227+
// Create the ad code.
228+
$ad_code_id = $this->acm->create_ad_code( $ad_code_data );
229+
$this->assertIsInt( $ad_code_id, 'Ad code should be created successfully.' );
230+
231+
// Register ad codes to make them available.
232+
$this->acm->register_ad_codes( $this->acm->get_ad_codes() );
233+
234+
// Enable display of ad codes without conditionals.
235+
add_filter( 'acm_display_ad_codes_without_conditionals', '__return_true' );
236+
237+
$args = array(
238+
'before_widget' => '<div class="widget">',
239+
'after_widget' => '</div>',
240+
'before_title' => '<h2>',
241+
'after_title' => '</h2>',
242+
);
243+
$instance = array(
244+
'title' => 'Ad Widget',
245+
'ad_zone' => $test_tag,
246+
);
247+
248+
ob_start();
249+
$this->widget->widget( $args, $instance );
250+
$output = ob_get_clean();
251+
252+
remove_filter( 'acm_display_ad_codes_without_conditionals', '__return_true' );
253+
254+
// Clean up.
255+
$this->acm->delete_ad_code( $ad_code_id );
256+
257+
$this->assertStringContainsString( '<div class="widget">', $output, 'Widget should output wrapper with valid ad code.' );
258+
$this->assertStringContainsString( '</div>', $output, 'Widget should output closing wrapper.' );
259+
}
260+
261+
/**
262+
* Test that acm_tag action is still fired (backwards compatibility).
263+
*
264+
* @covers ACM_Ad_Zones::widget
265+
*/
266+
public function test_acm_tag_action_is_fired(): void {
267+
$action_fired = false;
268+
$action_tag = null;
269+
270+
$action_callback = function ( $tag_id ) use ( &$action_fired, &$action_tag ) {
271+
$action_fired = true;
272+
$action_tag = $tag_id;
273+
};
274+
275+
add_action( 'acm_tag', $action_callback, 5 ); // Priority 5 to run before the default handler.
276+
277+
$args = array(
278+
'before_widget' => '<div class="widget">',
279+
'after_widget' => '</div>',
280+
'before_title' => '<h2>',
281+
'after_title' => '</h2>',
282+
);
283+
$instance = array(
284+
'title' => '',
285+
'ad_zone' => 'test_action_zone',
286+
);
287+
288+
ob_start();
289+
$this->widget->widget( $args, $instance );
290+
ob_get_clean();
291+
292+
remove_action( 'acm_tag', $action_callback, 5 );
293+
294+
$this->assertTrue( $action_fired, 'acm_tag action should be fired for backwards compatibility.' );
295+
$this->assertSame( 'test_action_zone', $action_tag, 'acm_tag action should receive the correct tag ID.' );
296+
}
297+
}

0 commit comments

Comments
 (0)