Skip to content

Handle singleton artifact objects per FAIR spec #497

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kasparsd wants to merge 6 commits into
base: release_1.5.0
Choose a base branch
from
+155 −9
Open

Handle singleton artifact objects per FAIR spec #497

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2a28a06
Normalize to arrays of artifact objects since spec supports both formats
kasparsd May 16, 2026
7bff913
Pick the best artifact for locale
kasparsd May 16, 2026
85afc80
Account for lang property being optional
kasparsd May 16, 2026
d5ca380
Add unit tests for artifact picker
kasparsd May 16, 2026
0e3a367
Describe the current state
kasparsd May 16, 2026
2d6b976
Merge branch 'release_1.5.0' into fix-package-artifacts-types
kasparsd Sep 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions inc/packages/class-releasedocument.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,15 @@ public static function from_data( stdClass $data ) {
}
}

// Normalize to arrays of artifact objects since spec supports both formats.
if ( $doc->artifacts instanceof stdClass ) {
foreach ( get_object_vars( $doc->artifacts ) as $type => $artifact ) {
if ( is_object( $artifact ) ) {
$doc->artifacts->{$type} = [ $artifact ];
}
}
}

return $doc;
}
}
24 changes: 15 additions & 9 deletions inc/packages/namespace.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -463,15 +463,19 @@ function pick_artifact_by_lang( array $artifacts, ?string $locale = null ) {
// Score artifacts based on match.
$score_artifact = function ( $artifact ) use ( $langs ) {
$score = 0;
$lang = strtolower( $artifact->lang ?? '' );

@kasparsd kasparsd May 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Account for missing lang property which is allowed per spec.


// Check for lang match.
$idx = array_search( strtolower( $artifact->lang ), $langs, true );
$idx = array_search( $lang, $langs, true );
if ( $idx !== false ) {
$score += ( count( $langs ) - $idx ) * 100;
}

return $score;
};

// Minimal behavior only: missing lang is tolerated, but equal-score ordering
// and partial-locale fallback precedence are intentionally unspecified.
usort( $artifacts, function ( $a, $b ) use ( $score_artifact ) {
$a_score = $score_artifact( $a );
$b_score = $score_artifact( $b );
Expand Down Expand Up @@ -738,6 +742,7 @@ function get_package_data( $did ) {
$type = str_replace( 'wp-', '', $metadata->type );
$sections = (array) $metadata->sections;
$description = trim( $sections['description'] ?? '' );
$package_artifact = isset( $release->artifacts->package ) ? pick_artifact_by_lang( $release->artifacts->package ) : null;

$response = [
'name' => $metadata->name,
Expand All @@ -759,8 +764,8 @@ function get_package_data( $did ) {
'new_version' => $release->version,
'version' => $release->version,
'remote_version' => $release->version,
'package' => $release->artifacts->package[0]->url,
'download_link' => $release->artifacts->package[0]->url,
'package' => $package_artifact->url ?? '',
'download_link' => $package_artifact->url ?? '',
'tested' => $required_versions['tested_to'] ?? '',
'external' => 'xxx',
'last_updated' => $metadata->last_updated ?? '',
Expand Down Expand Up @@ -803,10 +808,7 @@ function cache_did_for_install( array $options ): array {
$did = array_find_key(
$releases,
function ( $release ) use ( $options ) {
if ( ! is_array( $release->artifacts->package ) ) {
return false;
}
$artifact = pick_artifact_by_lang( $release->artifacts->package );
$artifact = isset( $release->artifacts->package ) ? pick_artifact_by_lang( $release->artifacts->package ) : null;

@afragen afragen May 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this return false for the check below?

@costdev costdev Sep 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionally speaking, null and false are both falsy so they'll both fail the truthy $artifact check below as expected.

Semantically speaking, this code is saying "The release artifact is..." (object|null), rather than "This has a release artifact" (bool), so null makes more sense to me when no release artifact exists.

Edit: pick_artifact_by_lang() returns stdClass|null, so this maintains a consistent fallback value type.

return $artifact && $artifact->url === $options['package'];
}
);
Expand Down Expand Up @@ -976,8 +978,12 @@ function maybe_add_accept_header( $args, $url ) : array {
}

foreach ( $releases as $release ) {
if ( $url === $release->artifacts->package[0]->url ) {
$content_type = $release->artifacts->package[0]->{'content-type'};
$artifact = array_find(
$release->artifacts->package ?? [],

@costdev costdev Sep 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR normalizes to an array of artifacts for a newly created ReleaseDocument via its ::from_data() method.

However, since the Transient API is a persistent cache mechanism, is may be possible that an existing pre-1.5.0 CACHE_RELEASE_PACKAGES transient value could still have the non-normalized package and therefore this array_find call could run on an object.

The normalization may need to run against $release->artifacts here before the array_find() call, and it may be worth abstracting the artifact normalization to a new static method on the ReleaseDocument class to reduce code duplication.

fn ( $package_artifact ) => $url === ( $package_artifact->url ?? '' )
);
if ( $artifact ) {
$content_type = $artifact->{'content-type'} ?? '';
if ( $content_type === 'application/octet-stream' ) {
$args = array_merge( $args, [ 'headers' => [ 'Accept' => $content_type ] ] );
break;
Expand Down
53 changes: 53 additions & 0 deletions tests/phpunit/tests/Packages/PickArtifactByLangTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
<?php
/**
* Tests for FAIR\Packages\pick_artifact_by_lang().
*
* @package FAIR
*/

use function FAIR\Packages\pick_artifact_by_lang;

/**
* Tests for FAIR\Packages\pick_artifact_by_lang().
*
* @covers FAIR\Packages\pick_artifact_by_lang
*/
class PickArtifactByLangTest extends WP_UnitTestCase {

// Edge cases intentionally not pinned down here: tie-breaking across multiple
// matching lang values, artifacts without lang, and fallback precedence across
// partial locale matches.

/**
* Test should prefer an exact locale match over artifacts without lang.
*/
public function test_should_prefer_exact_locale_match_over_artifact_without_lang() {
$fallback_artifact = (object) [
'url' => 'https://example.com/no-lang.zip',
];
$matching_artifact = (object) [
'url' => 'https://example.com/de-de.zip',
'lang' => 'de-DE',
];

$selected = pick_artifact_by_lang( [ $fallback_artifact, $matching_artifact ], 'de-DE' );

$this->assertSame( $matching_artifact, $selected, 'The exact locale match should be selected.' );
}

/**
* Test should not fail when artifacts do not specify lang.
*/
public function test_should_return_an_artifact_when_lang_is_missing() {
$first_artifact = (object) [
'url' => 'https://example.com/first.zip',
];
$second_artifact = (object) [
'url' => 'https://example.com/second.zip',
];

$selected = pick_artifact_by_lang( [ $first_artifact, $second_artifact ], 'de-DE' );

$this->assertContains( $selected, [ $first_artifact, $second_artifact ], 'Artifacts without lang should still return a valid artifact.' );

@costdev costdev Sep 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double-checking the reasoning why this checks that $selected is in an array rather than being a specific artifact. Is it because of inconsistent sorting across PHP versions or?

}
}
78 changes: 78 additions & 0 deletions tests/phpunit/tests/Packages/ReleaseDocumentTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
<?php
/**
* Tests for FAIR\Packages\ReleaseDocument.
*
* @package FAIR
*/

use FAIR\Packages\ReleaseDocument;

/**
* Tests for FAIR\Packages\ReleaseDocument.
*
* @covers FAIR\Packages\ReleaseDocument::from_data
*/
class ReleaseDocumentTest extends WP_UnitTestCase {

/**
* Test should normalize singleton artifact objects to arrays.
*/
public function test_should_normalize_singleton_artifact_objects_to_arrays() {
$package_artifact = (object) [
'url' => 'https://example.com/plugin.zip',
];
$icon_artifact = (object) [
'url' => 'https://example.com/icon.png',
];
$custom_artifact = (object) [
'url' => 'https://example.com/extra.json',
];

$release = ReleaseDocument::from_data(
(object) [
'version' => '1.2.3',
'artifacts' => (object) [
'package' => $package_artifact,
'icon' => $icon_artifact,
'x-extra' => $custom_artifact,
],
]
);

$this->assertNotWPError( $release, 'Expected a valid release document.' );
$this->assertIsArray( $release->artifacts->package, 'Package artifacts should be normalized to an array.' );
$this->assertIsArray( $release->artifacts->icon, 'Icon artifacts should be normalized to an array.' );
$this->assertIsArray( $release->artifacts->{'x-extra'}, 'Custom artifact types should be normalized to an array.' );
$this->assertSame( $package_artifact, $release->artifacts->package[0], 'The original package artifact should be preserved.' );
$this->assertSame( $icon_artifact, $release->artifacts->icon[0], 'The original icon artifact should be preserved.' );
$this->assertSame( $custom_artifact, $release->artifacts->{'x-extra'}[0], 'The original custom artifact should be preserved.' );
}

/**
* Test should preserve artifact arrays.
*/
public function test_should_preserve_artifact_arrays() {
$package_artifact = (object) [
'url' => 'https://example.com/plugin.zip',
];
$banner_artifact = (object) [
'url' => 'https://example.com/banner.png',
];

$release = ReleaseDocument::from_data(
(object) [
'version' => '1.2.3',
'artifacts' => (object) [
'package' => [ $package_artifact ],
'banner' => [ $banner_artifact ],
],
]
);

$this->assertNotWPError( $release, 'Expected a valid release document.' );
$this->assertCount( 1, $release->artifacts->package, 'Package artifact arrays should be preserved.' );
$this->assertCount( 1, $release->artifacts->banner, 'Banner artifact arrays should be preserved.' );
$this->assertSame( $package_artifact, $release->artifacts->package[0], 'Existing package array entries should be preserved.' );
$this->assertSame( $banner_artifact, $release->artifacts->banner[0], 'Existing banner array entries should be preserved.' );
}
}
Loading