10up / ads-txt

Ads.txt Manager for WordPress: Manage your ads.txt and app-ads.txt files in the WordPress dashboard
https://wordpress.org/plugins/ads-txt/
GNU General Public License v2.0
57 stars 25 forks source link

Check placeholder records and handle validation #129

Closed ankitrox closed 8 months ago

ankitrox commented 1 year ago

Description of the Change

Closes #78

How to test the Change

  1. Add the following record in ads.txt editor and save changes. You shall receive the warning "Your ads.txt indicates no authorized advertising sellers."

    placeholder.example.com, placeholder, DIRECT, placeholder
  2. Add following record in ads.txt editor. You will get the error "Line 2: Ads.txt contains placeholder record with another records."

    google.com, 4254, DIRECT
    placeholder.example.com, placeholder, DIRECT, placeholder

Changelog Entry

Added - Placeholder record can be added when no authorized sellers or buyers.

Credits

Props @peterwilsoncc

Checklist:

jeffpaul commented 1 year ago

@ankitrox any chance you're able to handle the code review item above?

ankitrox commented 1 year ago

@peterwilsoncc Thanks for the review :)

I wanted those couple of variables to be persistent across multiple function calls, but I agree that static may not be the idea ones as far as unit tests are concerned.

I have changed it to use options table in order for them to be persistent. This will also be handy for unit tests.

peterwilsoncc commented 1 year ago

I just tested this and am still seeing the no authorized resellers warning when the file contains both a placeholder and a valid record.

I did some testing to see if I could come up with an approach that worked but comment lines ended up suppressing the error if the file contained placeholders and comments (which is just as bad, if not worse).

My thoughts are:

We'll need to account for:

ankitrox commented 8 months ago

Thank you @peterwilsoncc

I have added the fix as per Peter's suggestion and it seems to be working fine. Also, added the unit test for the placeholder record.

CC: @jeffpaul