Allow empty ad sizes in responsive settings

I tested the patch, and it worked as expected.
It seems that we might want to make it more explicit, for better admin UX. The second patch keeps the validation in place, but revises it so that users can enter "<none>" in the Ad Size(s) field for any breakpoint at which the slot should be suppressed. It also revises various error messages and descriptive text to explain this new option.
I've been testing this, and so far it appears to be working nicely. I'd love it if someone else could verify and RTBC it.

I'll need to update the tests to take the new option into account...

Here's a new patch with an updated test (I didn't create a new test to verify that the "<none>" keyword is allowed and results in the expected javascript - I just updated an existing test to look for the new validation error message in its assertion).

Testing against #6

Functionally it works and the ads appear as expected. I would suggest adding some documentation to the field descriptions so it is obvious what is happening. Like This browser viewport and under will show the following ad position.

Also in UX the add another breakpoint functionality seems to not be functioning and causing some chaos in the text fields:
http://note.io/1FlJcXe

Looks like it is adding a second ad size text field and there is no delineation between where one breakpoint starts and another one begins. See screenshot above.

Worked against the latest DFP release.

Also in UX the add another breakpoint functionality seems to not be functioning and causing some chaos in the text fields:
http://note.io/1FlJcXe

Is there some additional module/styling applied to your settings? This change does not functionaly change the UI of the breakpoints, just the validation and processing.

I have also tested the patch in #6 and it works great.
I am not getting any issues with the "Add another breakpoint" UI button either.

Would be great to get this committed. :)

Unclear why, but we had some ads not appearing with the preg_replace in this patch. Changed:

-      $ad_sizes = preg_replace('/.*<none>.*/', '', $breakpoint['ad_sizes']);
+      $ad_sizes = strpos($breakpoint['ad_sizes'], '<none>') ? '' : $breakpoint['ad_sizes'];

Didn't mean to change status from RTBC. Patch should apply this time.

As a workaround I added a breakpoint with 1x1 which was not added to the size(s) field.

F.i. my sizes field has:
160x600,120x600

And my breakpoints are:
768x0 — 160x600,120x600
0x0 — 1x1

Only screens wider than or equal to 768px get the ad, smaller ones don’t.

$ad_sizes = strpos($breakpoint['ad_sizes'], '<none>') ? '' : $breakpoint['ad_sizes'];

should be

$ad_sizes = strpos($breakpoint['ad_sizes'], '<none>') !== FALSE ? '' : $breakpoint['ad_sizes'];

since strpos is going to return 0.

Rolling mrconnerton's fix into a patch.

I just came across this post, applied *-13.patch (manually) and have tested against the 7.x-1.5 codebase with multiple ad slots on my site.

Working as expected.

Is this intended on being rolled into the stable codebase sometime?

Thanks!

Re-rolled patch

Re-rolled again

Tested in 7.x-1.x branch and it's working as expected.

Re-rolled patch

New here, so sorry if this is a dumb question: is there any equivalent issue for this update on the Drupal 8 version of the DFP module? Or would this change also apply to the D8 code?

Tested most recent patch against latest 7.x-1.x version and works as expected

Tested patch from #23 against the dev and applies correctly. No errors and works as expected.

Just one little improvement to the tests

--- a/tests/dfp_display_tag.test
+++ b/tests/dfp_display_tag.test
@@ -69,6 +69,13 @@ class dfpDisplayTagTest extends dfpBaseTest {
     $edit['breakpoints[0][ad_sizes]'] = $this->randomName(8);
     $this->editTag($tag->machinename, $edit);
     $this->assertText(t('The ad size(s) string can only contain numbers, the character x and commas (unless it is the special keyword "!none").', array('!none' => htmlspecialchars('<none>'))), 'An error was correctly thrown when the Ad Size(s) field contained an invalid pattern.');
+
+    // Test tags with ad_size set to <none>.
+    $edit['breakpoints[0][browser_size]'] = '0x0';
+    $edit['breakpoints[0][ad_sizes]'] = '<none>';
+    $this->editTag($tag->machinename, $edit);
+    $this->drupalGet('');
+    $this->assertRaw('addSize([0, 0], [])');
   }

I can confirm that #29 works for me

I don't want to use dev version but want this patch.
I create patch from #29 and just remove changes to test file, so patch can now be applied to 1.5 version.