FEATURE: Respect globalScope and type, and find assets by metadata - #13
Draft
bwaidelich wants to merge 9 commits into
Draft
Conversation
Properties declared with `globalScope: true` now have a single value that is shared by all dimensions instead of the flag being ignored. Such values are stored under the reserved dimension hash `global`, so that for a given asset and property the storage holds either one shared value or one value per dimension space point, never both. Reads always look up the scope a property is configured for, which makes values of the respective other shape unreachable rather than wrong after a configuration change. The four read methods of the MetaDataManager are replaced by one, because they were three views of the same answer rather than three questions: `MetaDataPropertyValue` now carries the own value (for editing, without shine-through), the inherited value and its origin (for the translation hint) and the effective value (for rendering) side by side. This also halves the number of queries an inspector needs. Resolution rules move out of the storage: it no longer picks a winner via a MySQL specific `FIELD()` ordering but returns all values of the requested scope, leaving fallback priority to the MetaDataManager. Adds `assetmetadata:repair` to consolidate values whose scope contradicts the current configuration, plus unit and functional test suites.
Adds regression tests for 7d4c8f1: a configuration whose preset identifiers differ from the primary values of those presets, asserting that dimension space points are enumerated by value, that every enumerated point is considered valid and includes the default one, and that presets without values are skipped. All three fail when 7d4c8f1 is reverted.
globalScope of metadata properties
11 tasks
Adds `MetaDataManager::findAssets()`, returning the references of all assets that have a matching metadata value. The criteria of a `MetaDataAssetFilter` are all optional: asset source id, dimension space point, search term and the properties to search in. A value only counts if it is the one `getMetaDataPropertyValue()` would return for the filter's dimension space point, so the search agrees with what an editor working in that dimension sees: an asset whose caption is inherited from a fallback dimension is found, one whose inherited caption is overridden by a non matching value of its own is not. Properties with a global scope are matched on their shared value regardless of the dimension space point, just like they are read regardless of it. As everywhere else in this package, an omitted dimension space point means the *default* one rather than "any dimension" - a search is always carried out as seen from one dimension space point. Resolving the precedence per asset in PHP would mean a query per candidate, so it happens in SQL: the manager hands the storage the fallback chain ordered, from the most to the least specific dimension space point, and the storage applies that ranking rather than deriving one. The "order is meaningless" rule therefore moves from the interface docblock onto the method it actually describes. !!! `MetaDataStorage` gained a method, so third party implementations of that interface have to be extended.
Allows a single metadata property to be read from Fusion without fetching all
of them first:
caption = ${AssetMetaData.getMetaDataProperty(asset, 'caption', {language: 'de'})}
Like `getMetaData()` it returns the effective value, i.e. with dimension
fallbacks applied, and empty coordinates mean the default dimension.
`allowsCallOfMethod()` now allows every method, so that helper methods added
in the future are usable from Fusion without having to be listed twice.
Values used to be written and read as provided, so the `type` a property is declared with was exposed to consumers but never applied. It is now enforced in both directions: on the way in, so that nothing but a value of that type is ever stored, and on the way out, so that `MetaDataPropertyValue::$value` means what its `string|int|bool|null` signature says. Unambiguous conversions are applied, so that callers which only ever have strings - the command line, form input, Fusion - do not have to cast: "42" is a valid integer, "true", "on" and "yes" are a valid boolean, as are their negative counterparts. Anything else is rejected with an `InvalidArgumentException` rather than silently turned into a wrong value - "abc" is not 0. Reading is deliberately more forgiving, because it meets values that were written before a property was given its current type: a value that cannot be interpreted reads as NULL. Such a value is skipped rather than treated as an empty one, so that it does not shadow a fallback that is still readable. Booleans are stored as `1`/`0` and integers in decimal, which is what was already written before, so existing values stay readable.
There are two kinds of tests now: one for the `MetaDataManager`, with its dependencies as test doubles, and one per implementation of those dependencies. The manager tests no longer round trip through an in-memory storage. Each states the values the storage holds and asserts what resolves from them, so a test says what a rule *is* rather than demonstrating it through a write followed by a read. Writes are asserted as the calls they make. The repair tests work the same way and record those calls in order, which finally states outright that promotions happen before the deletions of the rows they came from. What a storage does with a lookup is its own business and is covered once per implementation. The tests of the DBAL adapter therefore also took over what used to be tested through the manager: the shadowing of values further down the fallback chain, the escaping of LIKE wildcards, distinctness, ordering and the separation of the localized and the global scope - plus the isolation of assets and asset sources, which was never a rule of the manager to begin with. They also cover the `MetaDataStorageMaintenance` surface that `assetmetadata:repair` is built on, including the values of unconfigured dimensions and undefined properties that reads can never return. `MetaDataConfigurationProviderYamlAdapter` had no test at all and has one now. The adapter stays MySQL specific, so its tests remain the only ones that need a database.
They describe where dimensions come from and what happens when the scope of a property is changed, so they belong to the configuration section as a whole rather than under the subsection about property types that was inserted above them.
globalScope of metadata propertiesglobalScope and type, and find assets by metadata
…a-properties FIX: Add getOwnValue method to MetaDataPropertyValue for fusion access
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Makes the property declarations do something —
globalScopeandtypewere both parsed and then read by nothing — collapses the read API, and adds a way to find assets by their metadata.Respect
globalScopeMetaDataPropertyDefinition::$globalScopewas parsed from the settings and then read by nothing. Properties declared withglobalScope: true—copyright, out of the box — were localized like any other, so the same asset could end up with a different copyright notice per language.Such properties now have exactly one value, shared by all dimensions. The dimension space point passed to a read or write is ignored for them, so callers never have to branch on
globalScope: the UI can pass whatever dimension it is currently working in.One read instead of four
The manager had four ways to read values, which existed because the UI needs three different answers:
getMetaDataPropertyValues()getMetaDataPropertyValuesOfParentWithFallback()getMetaDataPropertyValuesWithFallback()Those are three views of one answer rather than three questions — the third is just the first falling back to the second. They are now fields on the returned
MetaDataPropertyValue:Two side effects worth having:
inheritedFromtells the UI which dimension a hint comes from, so it can render “EN: A cat”. The old API returned bare values with no way to find out.For a property with a global scope the value is shared and therefore never inherited:
ownValueis that value andinheritedValueis alwaysNULL, which makes the inspector render it as a plain field with no hint line without special-casing anything.Respect
typetypewas the second half of the configuration that was parsed and then ignored — values were written and read exactly as provided, so anintegerproperty held whatever a caller happened to pass and always read back as a string.It is now enforced in both directions: on the way in, so that nothing but a value of that type is ever stored, and on the way out, so that
MetaDataPropertyValue::$valuemeans what itsstring|int|bool|nullsignature says.Unambiguous conversions are applied, so that callers which only ever have strings — the command line, form input, Fusion — do not have to cast:
stringintegerint, an optionally signed decimal string, or a booleanbooleanbool,"true"/"on"/"yes"/"1"and their negatives (any case), or1/01or0Anything else is rejected with an
InvalidArgumentExceptionrather than silently turned into a wrong value —"abc"is not0.Reading is deliberately more forgiving, because it meets values written before a property was given its current type: a value that cannot be interpreted reads as
NULL. Such a value is skipped rather than treated as empty, so it does not shadow a fallback that is still readable.Booleans are stored as
1/0and integers in decimal, which is what was already being written, so existing values stay readable.Finding assets
New:
findAssets()returns the references of all assets that have a matching metadata value. Every criterion of aMetaDataAssetFilteris optional and they are combined with AND:assetSourceIddimensionSpacePointsearchTermpropertyNamesThe interesting decision is what "matching" means. A value only counts if it is the one
getMetaDataPropertyValue()would return for the filter's dimension space point, so the search agrees with what an editor working in that dimension sees. Given an asset whose English caption isA cat, searching forcatin German finds it as long as German inherits that caption — and stops finding it the moment a German caption of its own is set. The cheaper alternative (OR over the whole chain) would keep matching a value that German never actually shows.Properties with a global scope are matched on their shared value regardless of the dimension space point, just as they are read regardless of it.
The result is lazily streamed, contains each asset at most once and is ordered by asset source id and asset id. It carries
MetaDataAssetReferences — the identity of an asset within its asset source — notAssetobjects; resolving those is up to the caller, this package never touches the asset model. There is deliberately no Media Browser wiring here and no pagination; a consumer intersects this with its own listing.AssetMetaData.getMetaDataProperty()The Eel helper could only fetch all properties. It now also reads a single one, which is what Fusion usually wants:
Storage stays dumb
The adapter used to pick the winner of a read itself, via
ORDER BY FIELD(dimension_hash, …)plusfetchOne(). "First match along the fallback chain wins" is a domain rule, so it moved to the manager: storage now returns every value of the requested scope, unordered, and the manager applies fallback priority.findAssets()is the one place where that could not hold. Resolving precedence per asset in PHP would mean a query per candidate, so the ranking has to be applied inside the query — via aNOT EXISTSanti-join that looks for a stored value closer along the chain. The manager therefore hands the storage the chain ordered, and the storage applies a ranking it was given rather than deriving one. The interface says so per method:getMetaDataPropertyValues()keeps "the order is meaningless here", the search method states the opposite explicitly.So
FIELD()is gone from the read path but back in the search, alongside<=>and the existingON DUPLICATE KEY UPDATE. The adapter stays deliberately MySQL-specific; the matching semantics of the search are those ofutf8mb4_unicode_ci(case- and accent-insensitive, verified against MySQL 8.0 rather than assumed).MetaDataStoragegainedfindAssets(). Listing and removing values regardless of scope, which only the repair command needs, lives in a separate optionalMetaDataStorageMaintenance; a storage that does not implement it still works and the command says so.Storage format and the scope invariant
No schema change, and none for the search either —
LIKE '%…%'cannot use an index, but the table holds at most (assets × properties × dimensions) rows. If that ever bites, aFULLTEXTindex is the way out and nothing in the public API would have to change.Shared values are stored under the reserved dimension hash
global— a real hash is always 32 hex characters, so the two cannot collide.For a given asset and property the table therefore holds either one shared value or one value per dimension space point, never both. That cannot be expressed as a constraint (it is a cross-row invariant), but it does not need to be: every read looks up the scope a property is configured for, so values of the other shape are simply unreachable. Flipping
globalScopecan never make stale values surface — and the search honours the same rule.assetmetadata:repairSince the shape of stored data is derived from mutable configuration, flipping
globalScopeleaves values behind that no longer match. They are invisible rather than harmful, so this is hygiene:--pruneis given. Pruning is refused outright while no content dimension is configured, because a broken or half-loaded dimension configuration would otherwise look exactly like every value being obsolete.This also cleans up after
assetmetadatamigration:migrateexistingassetproperties, which wrotecopyrightto the default dimension whilecopyrightships asglobalScope: true. The command itself needs no change — it routes through the manager and now lands on the shared value — but installations that already ran it are in the mixed state repair fixes.Breaking changes
MetaDataPropertyValuesnow maps a property name to aMetaDataPropertyValueinstead of a scalar;->toArray()gives the old shape. No consumer of any of this exists yet.MetaDataStoragechanged twice and third-party implementations have to follow: the read method returns all values of a scope instead of picking one, andfindAssets()was added. Putting the search behind another optional capability interface was considered and rejected — searching is core, not an extra.AssetMetaData.getMetaData()is unchanged for Fusion — it still returns a flat array of effective values.Three behaviour changes to be aware of:
assetmetadata:listwithout--dimension-space-pointnow applies fallbacks (it previously did not) and marks inherited values.AssetMetaData.getMetaData($asset)without coordinates used to throw in any dimensioned setup: it passed empty coordinates, whichisDimensionSpacePointValid()rejects whenever dimensions are configured. Empty coordinates now mean the default dimension.typenow throws instead of storing it. No shipped property is anything butstring, andstringaccepts anything, so no default configuration can hit this.Tests
The package had none, so this adds
require-dev,autoload-devand both suites. They are split along the seams the package is built on: one set for theMetaDataManagerwith its dependencies as test doubles, and one per implementation of those dependencies.MetaDataManagerTestMetaDataRepairTestMetaDataPropertyTypeTestMetaDataStorageProviderDbalAdapterTestMetaDataStorageimplementation — functionalDimensionSpacePointProviderContentRepositoryAdapterTestDimensionSpacePointProviderimplementationMetaDataConfigurationProviderYamlAdapterTestMetaDataConfigurationProviderimplementation110 unit tests, 215 assertions. They state the stored values they resolve from rather than writing them first, so a test says what a rule is instead of demonstrating it through a round trip. Repair records its calls in order, which lets it assert outright that promotions happen before the deletions of the rows they came from.
./bin/phpunit -c Build/BuildEssentials/PhpUnit/UnitTests.xml --filter 'Neos\\MetaData'30 functional tests, 53 assertions — the SQL that unit tests cannot reach: the upsert, lookup by scope, the shadowing anti-join, LIKE escaping, distinctness, ordering, and the
MetaDataStorageMaintenancesurface repair is built on. They are the only tests that need a database.They still do not run in this distribution. The functional bootstrap fails before loading any test, because
Flowpack\Media\Ui\Service\UsageDetailsServicerequires aContentRepositoryRegistrythat does not exist in Neos 8.4. It fails identically with a filter matching no test at all, so it is unrelated to this change. They were verified instead against MySQL 8.0 in a throwaway container — all 30 pass — but CI here has never executed them.Not covered: the repair command controller's output rendering. The repair logic itself is fully unit-tested through
MetaDataRepair.Review notes
MetaDataManager::resolvePropertyValue()is where the read rules come together, andMetaDataStorageProviderDbalAdapter::findAssets()is where the search does — worth reading first.TASK: Cover enumeration of dimension space points by preset valueadds regression tests for7d4c8f1on the base branch, which had none. They live here because the base branch has no test infrastructure at all; if that fix is merged independently, the tests do not travel with it.getMetaDataPropertyValues()still issues one query per property. It is pre-existing, unchanged by this PR, and worth an optional bulk-read interface once a consumer (a Media UI listing, say) actually feels it.Draft, because based on #11