License check separation and severity

[ernilambar]

Proposal 1:
Change error type

Code	Current Type	Proposed Type
invalid_license	WARNING	ERROR
license_mismatch	WARNING	ERROR

Proposal 2:
Currently same error code no_license is used for missing license in readme and plugin header which is confusing. Similar for invalid_license.

My proposal is keeping same error code for readme and add introduce separate error code for plugin header license check:

• plugin_header_no_license
• plugin_header_invalid_license

Proposal 3:
Regarding license check:

• Check one - Check if License is valid SPDX identifier (We could use error code like invalid_license_identifier and plugin_header_invalid_license_identifier)
• Check two - Check if License is GPL compatible (Use existing error code for this - invalid_license)

[ernilambar]

CC @frantorres @davidperezgar

[davidperezgar]

I see that separate checks could benefit to be more clear in the report. Ok for that.
Are your proposal separate? or making all of them? If it's the first thing, I'd vote for proposal 2, that is clear for the user.

[ernilambar]

All three would be good improvement.

[frantorres]

Regarding proposal 1:

invalid_license, as it is currently checked as a "whitelist" and probably not all GPL-compatible licenses are part of that list, if we have a case with an uncommon license we may have a false positive, but as for this year's experience this is really rare and in case it happens we can update that "whitelist", so I think it's fine to set it up as an ERROR, bearing in mind that there will be punctual exceptions that we will need to check and update the check when those happen.

license_mismatch I think it makes sense also to set it up as ERROR.

Regarding proposal 2: I agree to separate them, that will make it clearer.

Regarding proposal 3: A SPDX identifier might be fine but I wouldn't mark it as needed, plain text seems fine.

[ernilambar]

I am working for part 3 of the improvements proposal.

I dug down a little bit about license check. I found that after PR #454 our identifier check has been pretty relaxed and it passes almost every thing which was much stricter in the earlier implementation (and also in Legacy plugin).

Before above mentioned PR:

private function check_license( Check_Result $result, string $readme_file, Parser $parser ) {
	$license = $parser->license;

	if ( empty( $license ) ) {
		$this->add_result_error_for_file(
			$result,
			__( 'Your plugin has no license declared. Please update your readme with a GPLv2 (or later) compatible license.', 'plugin-check' ),
			'no_license',
			$readme_file
		);

		return;
	}

	// Test for a valid SPDX license identifier.
	if ( ! preg_match( '/^([a-z0-9\-\+\.]+)(\sor\s([a-z0-9\-\+\.]+))*$/i', $license ) ) {
		$this->add_result_warning_for_file(
			$result,
			__( 'Your plugin has an invalid license declared. Please update your readme with a valid SPDX license identifier.', 'plugin-check' ),
			'invalid_license',
			$readme_file
		);
	}
}

We used to check actual License value from readme unlike now we check normalized license value.

In PCP Legacy:

public function check_license_meets_requirements() {
  $license = $this->readme->license ?? '';

  // Cleanup the license identifier a bit.
  $license = str_ireplace( [ 'License URI:', 'License:' ], '', $license );
  $license = trim( $license, ' .' );

  if ( ! $license ) {
    return;
  }

  // Check for a valid SPDX license identifier.
  if ( ! preg_match( '/^([a-z0-9\-\+\.]+)(\sor\s([a-z0-9\-\+\.]+))*$/i', $license ) ) {
    return new Warning(
      'invalid_license',
      __( 'Invalid license specified.', 'plugin-check' ) . ' ' . sprintf(
        /* translators: 1: readme.txt */
        __( 'Your plugin has an invalid license declared. Please update your %1$s with a valid SPDX license identifier.', 'plugin-check' ),
        '<code>readme.txt</code>'
      )
    );
  }
}

I believe License identifier check should be more strict (at least in the level of strictness in PCP legacy).

@frantorres @davidperezgar

[davidperezgar]

Ok for that.