Repository navigation
Conversation
…moment Versions are numbered per locale, so "version 3" names a different state in every language. Nothing identifies one moment across locales, which a printed carrier needs: the passport state a product was placed on the market with is one moment, not one per language. Add `publication_releases`: one row per minted version, with a `sequence` that is monotonic per publication, minted inside the same lock as the version number. `publication_versions.release_id` points at it. Existing versions are backfilled, one release each in publish order. `PublicationRelease::versionsAsOf()` resolves the state as of a release: for every locale, the most recent version minted at or before it. That is the read a per-release public route needs and nothing else; this change alters no public behaviour. Releases are immutable like versions (update/delete throw), and `release_id` is sealed on the version.
… out of shared caches `Publisher::redactAll()` redacted only the `is_current` versions before flipping the publication to Redacted. Every superseded version kept its sealed payload readable in the database, so a GDPR Art. 17 erasure held only for as long as nothing ever read history. Now every not-yet-redacted version of the publication is redacted; versions already redacted on their own are left untouched. The public tombstone changes with it. A redacted passport answers `410 Gone` (the state is irreversible), while a withdrawn one stays `200` because it can be reinstated. Both tombstones are now `private, no-store` and `noindex`: the package has no cache purge hook, so a shared cache must never hold a tombstone that a reinstatement would have to displace, and the tombstone must not be indexed regardless of the channel's `indexable` setting. Tests: redactAll nulls superseded payloads and stamps the reason on them; a version redacted individually earlier keeps its own reason; the redacted route is 410 + no-store + noindex; the withdrawn route is 200 + no-store + noindex.
Adds `GET /{prefix}/{uuid}/r/{sequence}/{locale}`: the state of the
passport as of one release, for one locale, resolved through
`PublicationRelease::versionsAsOf()`. This is the URL a printed carrier
can be bound to: it names one moment across every language and keeps
resolving to exactly that state after the passport moves on.
Semantics:
- strict locale, no Accept-Language negotiation; a locale that had no
version yet at that release is 404 there
- 200 with a banner naming the release and whether it is still current,
linking to the live page; the locale switcher stays inside the release
- never indexed (`noindex, noarchive, nofollow`, a `<link rel=canonical>`
and a `Link` header pointing at the live page); no JSON-LD negotiation,
since a historical payload's stamped identity is the publication URL
- a version redacted individually renders the tombstone with 410 and
`no-store`, even while the publication itself stays Published; this
also closes the same gap on the live route
- the ETag now covers release, currency and redaction state, because
the banner and the tombstone change the HTML without changing the
checksum; TEMPLATE_VERSION bumped
- release pages are not counted as views
`show()` and `showRelease()` share one `render()`. The four new strings
live in the Publication package and are present in all 33 locales, in
English where no translation exists yet, so the translation audit passes.
|
Context, motivation and suggested review order for this and the related PRs: #683 |
The public `/carrier.svg` encodes the live URL, and nothing records what
was printed. A manual holds a QR code for years; when the passport moves
on there is no durable answer to "which code is in that manual and what
did it point at".
Add `publication_carrier_issuances`: publication, release, the exact
string encoded, format, issued_at, issued_by. Immutable, like versions
and releases. `CarrierIssuer::issue()` mints the row and derives the
target from the per-channel passport base URL, the same base every
other printed link uses.
The target is a new public entry route, `/{prefix}/{uuid}/r/{sequence}`:
it negotiates the locale once from Accept-Language among the locales
that exist in that release and 302s to the strict per-locale URL, the
same way the live carrier's bare `/{uuid}` works. Negotiation is moved
into `PublicationResolver::pickVersion()` so the live and release paths
share it.
Admin: "Issue QR code" per release on the versions page (publish
rights), returning the SVG as a download and recording the issuance.
The page also lists releases (with their locales) and every issuance
with its encoded link. QR rendering is shared via `CarrierSvg` so the
public live carrier and an issued release carrier print identically for
the same target.
A scanned GS1 link can carry the lot (AI 10) and serial (AI 21) that tie
a unit to the release it was placed on the market under. Add
`/01/{gtin}/10/{lot}`, `/01/{gtin}/21/{serial}` and the combination,
validated against the GS1 82-character set and 1-20 length in the
controller (a malformed qualifier is a bad link: 404, never a guess).
Which release a lot shipped under is not the PIM's knowledge, so the
mapping sits behind a `LotReleaseResolver` contract. The default binding
answers null and a qualified scan then resolves exactly like an
unqualified one, to the live passport. A consumer with batch or ERP
data rebinds it; when it names a release, the scan 302s to that
release's strict per-locale URL, with the locale negotiated once.
`publications.gtin` was overwritten on every publish, so a corrected
GTIN silently broke every `/01/{old}` link already printed. Add
`publication_gtins`, an append-only history written by
`SyncPublicationGtin` and backfilled from current values;
`PublicationResolver::findByGtin()` falls back to it when no publication
carries the scanned GTIN today, with the same designated-channel rule.
cadb84c to
31fbd24
Compare
* fix: validate empty files during product import instead of throwing code error (unopim#696) * fix: hide broken logo image in profile dropdown when remote fetch fails (unopim#700) * fix: disable @ attribute suggestions in profile image AI generation (unopim#701) * fix: set HTMLPurifier cache path to storage directory to prevent vendor write error during import (unopim#350) * fix: hide webhook logs tab and enforce ACL for unauthorized roles (unopim#545) * fix: EditImage tool now fetches product image by SKU instead of requiring upload (unopim#683) * fix: add XLSX export support to AI Agent ExportProducts tool (unopim#684) * fix: add SKU validation to AI Agent bulk import to reject special characters (unopim#689) * style: apply pint formatting to AiAgent lang file * fix: address copilot review on PR unopim#353
navneetkumar-pim-webkul
left a comment
There was a problem hiding this comment.
Thanks for this — the qualifier routes and the GTIN history close a real gap. Reviewed the new commit (31fbd24a) only; #679 → #680 → #681 → #682 are still open underneath and should land first, in order.
Must fix
- Scale: the migration backfill loads all
publicationsrows with an unboundedget()and inserts one row per iteration. On large catalogs this should stream (chunkById+ batchedinsert, or a singleinsertUsing). - Inline comments: the project keeps method/test bodies comment-free (see e.g.
Product/src/Repositories/ProductRepository.php). The rationale in the new//comments belongs in the method PHPDoc or the commit message. Flagged inline.
Should fix
- Deterministic old-GTIN fallback: when two publications have both carried a GTIN,
findByGtinWhere()can pick either one (noORDER BYwith a passport channel; achannel_idtie without one), and MySQL and PostgreSQL may disagree. Prefer the most recent carrier (recorded_at desc, id desc), plus a test. - No way to retire a wrong GTIN: history rows can't be updated or deleted, so a mistyped GTIN (possibly another brand's) resolves to this passport forever. A
revoked_atflag that the fallback skips keeps the table append-only while allowing a correction.
Minor
- A lot/serial containing
/is valid in the GS1 82-char set but 404s: the router decodes%2Fbefore matching[^\/]. SyncPublicationGtinrunsinsertOrIgnoreon every publish; it can skip when the GTIN is unchanged.- A resolver returning a release from another publication silently falls back to live — worth a
Log::warningso a buggy resolver is visible.
Looks good: bindIf + NullLotReleaseResolver as the extension point, 404 for malformed qualifiers instead of guessing, private, no-store + Vary: Accept-Language on the redirect, Concord proxy/contract + #[Fillable]/#[Table], explicit prefix-safe index names, portable insertOrIgnore, and good test coverage. CI is green on MySQL and PostgreSQL.
| $rows = DB::table('publications') | ||
| ->whereNotNull('gtin') | ||
| ->where('gtin', '!=', '') | ||
| ->get(['id', 'gtin', 'last_published_at', 'created_at']); |
There was a problem hiding this comment.
This loads every published row into memory, then inserts one row per loop iteration. On a large catalog, please stream it, e.g.:
DB::table('publications')
->whereNotNull('gtin')->where('gtin', '!=', '')
->select(['id', 'gtin', 'last_published_at', 'created_at'])
->chunkById(1000, function ($rows): void {
DB::table('publication_gtins')->insert($rows->map(fn ($row): array => [/* ... */])->all());
});(or a single insertUsing() from the select).
|
|
||
| $table->timestamps(); | ||
|
|
||
| // Explicit names: auto names include the prefix and overrun MySQL's 64-char identifier limit on prefixed installs. |
There was a problem hiding this comment.
Nit: the project keeps method bodies comment-free. Could this rationale (and the backfill note on L29) move to the commit message or a docblock?
| } | ||
|
|
||
| // A GTIN the publication carried earlier: carriers printed under it must keep resolving after a correction. | ||
| $previouslyOwned = PublicationGtinProxy::modelClass()::query()->where('gtin', $gtin)->pluck('publication_id'); |
There was a problem hiding this comment.
If more than one publication has carried this GTIN (e.g. it moved A → B, then B was corrected away too), findByGtinWhere() ends in ->first() without an ORDER BY when a passport channel is configured, or with a channel_id tie otherwise. The result is then engine-dependent. Suggest resolving to the most recent carrier (order by publication_gtins.recorded_at desc, id desc), with a test for this case. The comment on L55 could also move into the PHPDoc.
| throw new ImmutableVersionException('GTIN history row '.$row->id.' is immutable.'); | ||
| }); | ||
|
|
||
| static::deleting(function (self $row): void { |
There was a problem hiding this comment.
Append-only makes sense, but there's currently no way to retire a GTIN that was published by mistake (e.g. a typo that belongs to another brand): it will resolve to this passport forever. A nullable revoked_at that the fallback lookup skips (plus a small command or admin action to set it) would keep the history immutable while allowing a correction.
| $model::query()->whereKey($publication->id)->update(['gtin' => $gtin]); | ||
|
|
||
| // Append-only history: a `/01/{gtin}` link printed under an earlier GTIN must keep resolving after a correction. | ||
| PublicationGtinProxy::modelClass()::query()->insertOrIgnore([ |
There was a problem hiding this comment.
Minor: this runs a write on every publish even when the GTIN hasn't changed. It could be skipped when $publication->gtin === $gtin. Also, the comment on L49 would read better in the class docblock.
| // bounds the segment here; the 82-character set and 1-20 length are enforced in the controller. | ||
| foreach (['/01/{gtin}/10/{lot}' => 'gs1.lot', '/01/{gtin}/21/{serial}' => 'gs1.serial', '/01/{gtin}/10/{lot}/21/{serial}' => 'gs1.lot.serial'] as $uri => $name) { | ||
| Route::get($uri, [PublicationController::class, 'resolveByGtinQualified']) | ||
| ->where(['gtin' => Gs1DigitalLink::GTIN_PATTERN, 'lot' => '[^\/]{1,80}', 'serial' => '[^\/]{1,80}']) |
There was a problem hiding this comment.
/ is part of the GS1 82-character set and isWellFormedQualifier() accepts it, but Laravel matches routes against the decoded path, so a serial like A%2F1 never reaches the controller and 404s. Fine to accept that as a limitation, but maybe worth a note/test either way. The comments on L157–158 and L42 could also move to a docblock.
| ->assertRedirect('/p/'.$publication->uuid.'/r/1/'.$versions[0]->locale->code) | ||
| ->assertHeader('Vary', 'Accept-Language'); | ||
|
|
||
| // Unknown lot: the resolver answers null, so the scan lands on the live passport rather than a dead end. |
There was a problem hiding this comment.
Nit: the test name or a separate it() could carry this instead of an inline comment.
The backfill loaded every publication with a GTIN through one unbounded get() and inserted one history row per iteration, which does not scale on large catalogs. Read the publications with chunkById and insert one batch per chunk, so memory stays flat and the number of statements drops by the chunk size. The explanatory comments in the migration body move into the PHPDoc of up().
The project keeps method and test bodies comment-free. Move the rationale added with the GS1 qualifiers into the PHPDoc of the method or class it explains (service provider register() and registerPublicRoutes(), SyncPublicationGtin, PublicationResolver::findByGtin()). In the qualifier test, the unknown-lot case becomes its own it() so its name carries the intent.
When more than one publication has carried the same GTIN (it moved from A to B, then was corrected away from B too), the history fallback in findByGtin() ended in first() without an ORDER BY when a passport channel was configured, or in a channel_id tie otherwise, so the result depended on the database engine. Order the history by recorded_at desc, id desc and pick the first candidate that passes the channel rule in PHP. The designated passport channel still restricts the candidates, and the warning for the undesignated case is kept. A data-provider test covers a later carrier on the higher channel, on the lower channel, and an identical instant.
The GTIN history is append-only, so a GTIN published by mistake (a typo
that belongs to another brand, say) kept resolving to the passport that
once carried it. Add a nullable revoked_at to publication_gtins that the
history fallback skips, keeping the table append-only: PublicationGtin
now permits exactly one change, setting revoked_at on a row that is not
revoked yet, via revoke().
`unopim:publication:revoke-gtin {gtin} {--publication=}` retires the
active history entries for a GTIN. It refuses a malformed GTIN and leaves
alone a publication that still carries the GTIN today, since correcting
that is a republish. Revoking never touches publications.gtin, so a
publication that publishes the GTIN again resolves through its current
value as before.
The column joins the create migration of the same pull request, which has
not shipped yet.
SyncPublicationGtin ran an update and an insertOrIgnore on every publish. Both only matter when the GTIN differs from the one the publication already carries, so skip them when it does not. The canonical alias handling still runs.
…'s release A LotReleaseResolver that names a release of a different publication is a bug in the consumer's resolver. The controller already refuses to follow it and falls back to live, but did so silently. Log a warning with the resolver class, the publication and the release so the bug is visible. Also document in the PHPDoc that a lot or serial containing `/` does not resolve: it is valid in the GS1 82-character set, but the router matches the decoded path, so a `%2F` splits the segment and the link 404s.
…nd encoded slashes Pin the three small behaviours of the qualifier routes and the GTIN sync: a publish with an unchanged GTIN performs no history or GTIN write, a resolver returning another publication's release logs a warning and lands on live, and a serial with an encoded slash 404s (a documented limitation of the route grammar).
|
Thanks for the thorough review, much appreciated. All points are addressed on Must fix
Should fix
Minor
Publication and ProductPassport suites pass on MySQL 8.0 and PostgreSQL 16, both on the branch and on a trial merge with current 3.x. Pint and PHPStan are clean. Could you take another look when you have a moment? |
1 similar comment
|
Thanks for the thorough review, much appreciated. All points are addressed on Must fix
Should fix
Minor
Publication and ProductPassport suites pass on MySQL 8.0 and PostgreSQL 16, both on the branch and on a trial merge with current 3.x. Pint and PHPStan are clean. Could you take another look when you have a moment? |
Stacked on #682 (→ #681 → #679 + #680)
Only the last commit (
feat(publication): GS1 Digital Link qualifiers and GTIN history) is new here.Problem
Two gaps on the GS1 side of the same lifetime-reachability track.
/01/{gtin}is the only shape the resolver accepts, so that information is dropped and every scan lands on the live state.SyncPublicationGtinoverwritespublications.gtinon every publish. A corrected GTIN therefore silently breaks every/01/{old-gtin}link already printed:findByGtin()finds nothing and the scan 404s.Change
Qualifier routes:
/01/{gtin}/10/{lot},/01/{gtin}/21/{serial}and/01/{gtin}/10/{lot}/21/{serial}(GS1 order). The router only bounds the segment; the GS1 82-character set and 1–20 length are enforced in the controller viaGs1DigitalLink::isWellFormedQualifier(). A malformed qualifier is a bad link and 404s; it never falls through to a guess.LotReleaseResolvercontract (resolve(Publication, ?lot, ?serial): ?PublicationRelease). Which release a lot shipped under is not the PIM's knowledge, so the engine defines the question and bindsNullLotReleaseResolverby default (bindIf, so a consumer with batch or ERP data can rebind). With the default, a qualified scan resolves exactly like an unqualified one. When a resolver names a release, the scan 302s to that release's strict per-locale URL (locale negotiated once fromAccept-Languageamong the locales in that release).resolveByGtin()now delegates to the qualified path with null qualifiers, so both share one implementation.publication_gtins: append-only history of every GTIN a publication has published under, written bySyncPublicationGtin(insertOrIgnore, unique per publication + GTIN) and backfilled from current values in the migration.PublicationResolver::findByGtin()first looks up the current GTIN as before; if nothing carries it today, it falls back to publications that carried it earlier, applying the same designated-passport-channel rule. Rows refuse update and delete.No lang changes. PHPStan (level 2) clean.
Tests (
Gs1QualifierTest, ProductPassport suite because only the real builder emitsidentifier.gtin)L1302s to/p/{uuid}/r/1/{locale}withVary: Accept-Language; an unknown lot lands on the live passport|in a serial 404publications.gtinis the new value, the history holds both, and both/01/{old}and/01/{new}resolve to the same passportExisting GS1, carrier-target and check-digit tests pass unchanged.
Related
Closes the track described in #683, after #677, #678, #679, #680, #681 and #682. Remaining follow-ups noted there: an ETag on the live
/carrier.svg, and JSON-LD for historical states once a version-level identity is stamped into the payload.