This will need to ensure that '[src]' becomes 'data-amp-bind-src', so replacing the first [ with data-amp-bind- and stripping the last ].

Ah, good point.

Applied in 39926b6...bcec1fa

This should get generated as 'data-amp-bind-src'. See https://github.com/ampproject/amp-wp/pull/4285/files#r378581454.

Good idea, it's applied in 39926b6...bcec1fa

I know this isn't really something you can answer, but something seems off about the data format in the validator spec:

  attrs: {
    name: "src"
    mandatory_oneof: "['src', 'srcdoc']"
    value_url: {
      protocol: "data"
      protocol: "https"
      allow_relative: true  # Will be set to false at a future date.
    }
    blacklisted_value_regex: "__amp_source_origin"
  }
  attrs: {
    name: "srcdoc"
    mandatory_oneof: "['src', 'srcdoc']"
  }

It's also somewhat strange that "['src', 'srcdoc']" is serialized as pseudo-JSON, as opposed to being represented as an actual array in the protoascii.

Why is mandatory_oneof duplicated in both srcdoc and src?

In other words, it would seem more logical for the mandatory_oneof property to be placed under the tag_spec as opposed to duplicated inside of each attribute. For example:

diff --git a/includes/sanitizers/class-amp-allowed-tags-generated.php b/includes/sanitizers/class-amp-allowed-tags-generated.php
index b60135580..1aae2b432 100644
--- a/includes/sanitizers/class-amp-allowed-tags-generated.php
+++ b/includes/sanitizers/class-amp-allowed-tags-generated.php
@@ -3290,6 +3290,10 @@ class AMP_Allowed_Tags_Generated {
 					),
 				),
 				'tag_spec' => array(
+					'mandatory_oneof' => array(
+						'src',
+						'srcdoc',
+					),
 					'amp_layout' => array(
 						'supported_layouts' => array(
 							6,

Would it make sense to gather up the mandatory_oneof list across all attributes and then store them once in the tag_spec?

It turns out the value of the mandatory_oneof is just an opaque string that is used for the error message. The validator doesn't actually parse it. See the logic in the JS validator: https://github.com/ampproject/amphtml/blob/5fb4e7bb72012c2e088ffda951c3b815b6e1caba/validator/engine/validator.js#L4732-L4761

That is why the string is duplicated across the attributes, since it is just a key.

So in theory, instead of:

  attrs: {
    name: "src"
    mandatory_oneof: "['src', 'srcdoc']"
    value_url: {
      protocol: "data"
      protocol: "https"
      allow_relative: true  # Will be set to false at a future date.
    }
    blacklisted_value_regex: "__amp_source_origin"
  }
  attrs: {
    name: "srcdoc"
    mandatory_oneof: "['src', 'srcdoc']"
  }

The protoascii could have factored out the mandatory_oneof like so:

  attrs: {
    name: "src"
    value_url: {
      protocol: "data"
      protocol: "https"
      allow_relative: true  # Will be set to false at a future date.
    }
    blacklisted_value_regex: "__amp_source_origin"
  }
  attrs: {
    name: "srcdoc"
  }
  mandatory_oneof: ['src', 'srcdoc']

If we represented the data in this way, this would eliminate the current duplication of checking the same constraints across all attributes that have the same mandatory_oneof value in a given tag.

Otherwise, the only way to prevent the duplication now would seem to be to replicate what the JS validator is doing and instead of checking check_attr_spec_rule_mandatory_number_of inside the loop that iterates over the attributes, to instead check it once after they have been iterated over.

Nevertheless, it seems better to improve the data model by factoring out the duplicated data from the attributes and into the tag_spec.

Yeah, that's a good idea to remove the duplication in the generated PHP file. I'll work on applying it.

969def4 updates the generated file, but of course I need to update the PHP logic.

I have a suggestion to improve the logic here which would eliminate the need for GetMandatoryOfAttr. Given that the value is not actually parsed, what can be done is merely to check if the key exists for a given attribute, and of so, add the attribute name to the list for the constraint.

So this could be replaced with:

attributes = []
for attr_spec in attr:
	if attr_spec.HasField(constraint):
		attributes.append( attr_spec.name )
if len( attributes ) == 0:
	return None
return attributes

And then GetMandatoryOfAttr could be removed.

Is that right?

Thanks, that's a great idea.

3b1dbf8 applies it.

Hm, this looks to be the same as the mandatory_oneof spec below:

'mandatory_oneof' => array(
	'src',
	'srcdoc',
),

Oh, that's bizzare. How is Python generating an array with duplicate keys?

				'tag_spec' => array(
					'mandatory_oneof' => array(
						'src',
						'srcdoc',
					),
					'amp_layout' => array(
						'supported_layouts' => array(
							6,
							2,
							3,
							7,
							9,
							1,
							4,
						),
					),
					'mandatory_oneof' => array(
						'src',
						'srcdoc',
					),
					'requires_extension' => array(
						'amp-iframe',
					),
				),

I just re-ran ./bin/amphtml-update.sh and it removed the duplicate: 4fb49da.

Yeah, that's a little strange.

Good idea, having MISSING in the error code when there are too many is misleading.

Good idea to have this return the intersection.

This looks good, it's much more descriptive than before: