Check placeholder records and handle validation

[ankitrox]

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:

• I agree to follow this project's Code of Conduct.
• I have updated the documentation accordingly.
• I have added tests to cover my change.
• All new and existing tests pass.

[peterwilsoncc]

@ankitrox Thanks for the pull request.

Rather than use a static, would it be possible to check if both placeholder and valid records exist within the save function?

ads-txt/inc/save.php

Lines 34 to 46 in a00bdce

	foreach ( $lines as $i => $line ) {
	$line_number = $i + 1;
	$result = validate_line( $line, $line_number );
	$sanitized[] = $result['sanitized'];
	if ( ! empty( $result['errors'] ) ) {
	$errors = array_merge( $errors, $result['errors'] );
	}
	if ( ! empty( $result['warnings'] ) ) {
	$warnings = array_merge( $warnings, $result['warnings'] );
	}
	}

Statics can be difficult to check and while testing the changes with the test_validate_line function, I discovered the static was affecting other tests.

[jeffpaul]

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

[ankitrox]

@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]

I've added quite a few suggestions inline to avoid thrashing the database with option updates.

To record the line number, I've switch from an option to a static variable. (This may need further thought)

To record the presence of a placeholder, I've added a value to the returned array in validate_line().

While testing, I noticed that the order of lines affects the warnings shown. If I place a valid record above the placeholder record I am shown the warning "Your ads.txt indicates no authorized advertising sellers."

If I place the two lines in the reverse order, I also get a message that "Ads.txt contains placeholder record with another records."

[peterwilsoncc]

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:

• rename has_placeholder_records to has_only_placeholder_records, in the validate_file function set it to null
• if we find a valid line, set the renamed variable to false
• if we find a placeholder and the existing value is null, set it to true.
• show the warning if the value is either null or true

We'll need to account for:

• comments: no affect on the value
• empty lines: no affect on the value
• invalid records: no affect on the value
• valid record
• placeholders

[ankitrox]

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

[peterwilsoncc]

This looks good to me and tests well. Thank you so much for your patience as we got this working.

I pushed a couple of minor changes:

• 811fabc to make sure the optional tag value was set
• f1a56e5 to remove some sniff suppression that were not needed.