See #3563380-6: Allow linking integer timestamps to `type: string, format: date`: allow `DynamicPropSource` to use a single-input adapter plugins โ and #3563380-7: Allow linking integer timestamps to `type: string, format: date`: allow `DynamicPropSource` to use a single-input adapter plugins โ : that's working now! Generic solution is for ๐ [later phase] When generating suggestions for ContentTemplates, for any entity field property not appearing in any suggested `DynamicPropSource`, try to find an adapter to suggest it anyway Postponed .
Keeping self-assigned for the remainder of #24.
Follow-up created: ๐ [later phase] When generating suggestions for ContentTemplates, for any entity field property not appearing in any suggested `DynamicPropSource`, try to find an adapter to suggest it anyway Postponed .
Needs tests: https://git.drupalcode.org/project/canvas/-/merge_requests/433#note_642498 + https://git.drupalcode.org/project/canvas/-/merge_requests/433/diffs#not...
wim leers โ created an issue.
Implemented.
Still works as in the GIF in the comment above.
Note: to make this work in code components, code components will need to actually offer type: string, format: date as a choice, which they currently do not:
Off until 2026. See you January 2! ๐
#4 presumes we need to solve everything, generically.
What if we start smaller: ONLY the functionality described here?
But โฆ what if we start smaller? What if we make only the created and changed fields on Nodes work, and defer the generic solution to later? Supporting "created" and "changed" seems a crucial first step, right?
IOW:
โ Realized that in principle thanks to the work @jessebaker and I did with the one-liner in
๐
Linking and unlinking a field does not correctly reset the form view to it's initial state
Active
, now the promise of โUI does not need to understand prop sources, treats it as opaque blobsโ should be upheld.j
And โฆ it just works:
What's the catch?
The benefit would be none of the new infrastructure described in the comment before this one and in
#3548298-25: Support linking field types marked SUPPORTED from #3512433 to props in a `ContentTemplate` โ
: no changes to Component config entities at all.
The downside would be that we would need to start allowing AdaptedPropSource in ContentTemplates. Which opens a very broad set of things. At least in theory: people could manually hack their ContentTemplate config entities to start using various adapters.
There's 3 ways we could lock that down:
- Make
AdaptedPropSourceitself throw an exception in non-test contexts when using any adapter other than\Drupal\canvas\Plugin\Adapter\UnixTimestampToDateAdapter - Introduce a new
AdaptedDynamicPropSource, which only accepts single-input adapters (or even just the UNIX-to-date adapter) - Introduce a new feature on
DynamicPropSourceitself, that applies a single adapter plugin (or even initially restricted to just the UNIX-to-date adapter).
Thoughts? I prefer the second and third options, because it keeps the super broad impact and potential of "combine many inputs into a single output" and "chain multiple adapters" confined to AdaptedPropSource.
Discussed with @penyaskito. He prefers choice #3. Retitling accordingly.
I defer to our Code Clarity Chief, @phenaproxima.
So: consider this a pre-emptive RTBC ๐
Ugh, I updated the contribution record and updated the commit message per the "conventional commits" style, but failed to make @penyaskito the main author. He definitely was. Sorry, @penyaskito!
Interesting report, thanks!
_admin_route corresponds to:
- "needs admin theme" โ
\Drupal\user\Theme\AdminNegotiator, which does NOT apply to Canvas - "is admin context" โ
\Drupal\Core\Routing\AdminContext, which is arguably not the case when creating/editin (Canvas) landing pages, developing code components etc - "needs admin permissions" โ also does not apply
Despite all that, I do see your point!
The problem is that the meaning of "admin route" is not very well defined by Drupal core itself. But maybe you can point to the right spot? ๐๐
Related:
๐
Consider using the administration theme when editing or creating content by default
Active
aka
https://www.drupal.org/node/3514929 โ
โ creating/editing nodes now always use the "admin theme". See how \Drupal\node\EventSubscriber\NodeAdminRouteSubscriber uses that setting. Given that, I think what this proposed probably makes sense?
Let's give it a try and see what fails? That will help us make a decision?
Per #3549726-18: Remove `category` from `Component` config entities, add `ComponentSourceInterface::determineDefaultFolder()` instead โ , that issue will have implemented #11!
That makes the remaining scope here smaller still.
I think this means the first half of
๐
Expand Folder entity functionality
Active
becomes obsolete: it's already done. ::testFolderAutoCreationValidation() and ARIA tree expectation changes prove that.
Only nits, looks amazing!
Are we sure? โจ Add documentation for using SDC with Experience Builder Active literally changed it to this in https://git.drupalcode.org/project/canvas/-/merge_requests/69 ๐ฌ
Retitling to comply with our Code Clarity Chief's preferences ๐ค /me waves to @phenaproxima
๐ Remove the ability for component instances to deviate from the field type + storage + instance settings dictated by the `Component` version Active is addressing #10. ๐ Once that lands, this will be easy to do! ๐
RTBC as far as I'm concerned โ only nits need processing at this point, see https://git.drupalcode.org/project/canvas/-/merge_requests/358#note_642289
Paired with @penyaskito about his one concern: that I would have concerns about one particular thing, which I did ๐ We now have a path forward! ๐
Canvas already has \Drupal\canvas\Plugin\Adapter\UnixTimestampToDateAdapter, with this annotation:
#[Adapter(
id: 'unix_to_date',
label: new TranslatableMarkup('UNIX timestamp to date'),
inputs: [
'unix' => ['type' => 'integer'],
],
requiredInputs: ['unix'],
output: ['type' => 'string', 'format' => 'date'],
)]
This was added as a very rough PoC in July 2024, in ๐ Centralize & standardize logic for constructing *PropSource objects + kernel test coverage Fixed .
An immediately obvious problem is that type: integer is far too broad. Two integers might be describing wildly different things: just a number, a year, a timestamp, a count, a measure (weight in kg or pounds, length in cm or inches, distance, etc.), dimensions (width/height), a ratio โ you name it!
If we don't know the meaning of an integer, we won't be able to correctly line things up: Canvas would offer "3 [teaspoons]" as a way to generate a date, which is of course absurd. This is why we need to know the semantics. We did the same for strings. We'll also need to do this for integers: ๐ Add `IntegerSemanticsConstraint` Active .
Note this is already a major WTF for ContentTemplates: any SDC whose prop has type: integer as the schema gets the most absurd range of suggestions: from author picture width and file size to the last login of the user who created the current revision ๐คช This is what the issue summary of #3533675 captures.
- integer (and number) semantics:
๐
Add `IntegerSemanticsConstraint`
Active
, because otherwise even if we have adapters, we'll get nonsensical suggestions when using the Canvas
ContentTemplateUI - supporting Canvas' "adapter plugins" transparently โ without a single change to the UI or storage: we want to avoid the (even more experimental, because it predates the use of drupal.org issues for XB/Canvas!)
AdaptedPropSourceto end up being stored in our various component trees - Nope, not actually a problem, because
โจ
Real-time preview for props changes of JS components
Active
applies only to
StaticPropSources, not linkedDynamicPropSources! ๐
Of these:
- Is to avoid nonsense (which was identified long before ๐ฑ [META] Content templates Active even began!) โ ready for review: #3533675-16: Avoid suggesting UNIX timestamp integers for `type: integer` props โ
- Is to avoid disruption/risk โ expect MR tomorrow
I prefer approach #2, but I'd love to hear @phenaproxima's take ๐
While implementing #8, I realized the existing \Drupal\canvas\JsonSchemaInterpreter\JsonSchemaType::toDataTypeShapeRequirements() logic/infra and the DataTypeShapeRequirement value object cannot express "the NumbersSemanticsConstraint, with any of the semantics/units, EXCEPT this one".
So, added a simple negate flag to DataTypeShapeRequirement, and a one-line tweak to the logic and โฆ ๐
MR 426 is green! ๐
Not quite satisfied โ alternative approach!
However, I'm not quite satisfied with the "number semantics constraint" overall: I'm not convinced it will stand the test of time.
While finishing !426, I realized there's an alternative approach, that allows us to postpone temporarily (or maybe indefinitely) the "number semantics" constraint โ hopefully until after the JSON Schema folks get their system to a better place ๐ค
\Drupal\Core\Field\Plugin\Field\FieldType\TimestampItem contains
"value" => [
"Range" => [
"min" => "-2147483648",
"max" => "2147483648",
],
],
ever since @Berdir added that over 11 years ago at #2226493-168: Apply formatters and widgets to Node base fields โ !
We can reuse 90% of what !426 did, negate that instead, and avoid adding a new constraint altogether! However, that made me run into yet another core bug โฆ which has been fixed in fixed in 11.3 ~2 months ago! See ๐ CreatedItem and ChangedItem are missing TimestampItem's Range constraint Active .
Wow! I just ran into this while working on ๐ Add `IntegerSemanticsConstraint` Active ! So nice to see this fixed already ๐คฉ
Retitling to reflect the actual purpose.
From #6 + #7, I would propose this simplified subset:
#[Constraint(
id: 'NumberSemantics',
type: [
'integer',
'float',
'decimal',
],
)]
final class NumberSemanticsConstraint {
/** Pure number, no physical meaning โ unit forbidden. */
public const RAW = 'raw';
/** Identifier โ unit forbidden. */
public const ID = 'id';
/** Count of items, events, bytes, pixels, people, โฆ โ unit optional. */
public const COUNT = 'count';
/** 0-based index, 1-based ordinal, page number, ranking, โฆ โ unit optional. */
public const POSITION = 'position';
/** Any of https://en.wikipedia.org/wiki/List_of_physical_quantities and https://en.wikipedia.org/wiki/Unit_of_measurement โ โ ๏ธ UNIT REQUIRED (km, km/h, mi, lbs, EUR, USD โฆ). */
public const MEASURE = 'measure';
/** Calendar year, month, day, hour, minute, second, Unix epoch, โฆ โ โ ๏ธ UNIT REQUIRED (year, epoch-unix). */
public const MEASURE_TIME_POINT = 'measure-time-point';
/** Duration, age, timeout, TTL, โฆ โ โ ๏ธ UNIT REQUIRED (seconds/years/โฆ). */
public const MEASURE_TIME_SPAN = 'measure-time-span';
/**
* One of the above constants.
*/
public readonly string $semantic;
/**
* A unit, probably, depending on the semantic.
*/
public readonly ?string $unit;
}
IOW: 5 basic semantics:
- raw
- ID
- count
- position
- measure
In principle the MEASURE_ ones are not necessary, because the unit would reveal enough. So perhaps we should omit those too? ๐ค
Let's assume that.
How this would get used
To ensure all "image" fields' width and height field properties get the appropriate semantics, we'd add this to \Drupal\canvas\Plugin\Field\FieldTypeOverride\ImageItemOverride::propertyDefinitions():
$properties['width']->addConstraint('NumberSemantics', ['semantic' => NumberSemanticsConstraint::MEASURE, 'unit' => 'px']);
$properties['height']->addConstraint('NumberSemantics', ['semantic' => NumberSemanticsConstraint::MEASURE, 'unit' => 'px']);
And to ensure all "created", "changed" and "timestamp" fields on entities we'd add this to \Drupal\canvas\Hook\ShapeMatchingHooks::entityBaseFieldInfoAlter():
foreach ($fields as $field_definition) {
$is_unix_timestamp = is_subclass_of($field_definition->getItemDefinition()->getClass(), TimestampItem::class);
if ($is_unix_timestamp) {
$field_definition->addPropertyConstraints('value', 'NumberSemantics', ['semantic' => NumberSemanticsConstraint::MEASURE, 'unit' => 'epoch-unix']);
}
}
โ This would allow Canvas to automatically never suggest image width/height as inputs for \Drupal\canvas\Plugin\Adapter\UnixTimestampToDateAdapter, but only "created" and "changed" fields on entities, as is needed for
โจ
Allow linking integer timestamps to `type: string, format: date`
Active
.
Missing/out of scope here
To make that last sentence true, we would require richer metadata for \Drupal\canvas\Plugin\Adapter\UnixTimestampToDateAdapter, because that currently only has:
#[Adapter(
id: 'unix_to_date',
label: new TranslatableMarkup('UNIX timestamp to date'),
inputs: [
'unix' => ['type' => 'integer'],
],
requiredInputs: ['unix'],
output: ['type' => 'string', 'format' => 'date'],
)]
That would need to gain:
<code>
โฆ
inputs: [
'unix' => ['type' => 'integer'],
],
inputsSemantics: [
'unix' => ['NumberSemantics', ['semantic' => NumberSemanticsConstraint::MEASURE, 'unit' => 'epoch-unix']],
],
โฆ
)]
(which would have to be an optional new parameter of this annotation)
This is necessary because JSON Schema has no way to express semantics for type: integer and type: number, unlike type: string, where at least some semantics are definable (using format).
(Note that per one spec, one could write type: integer, format: utc-millisec, but in the official one, that is not a thing at all. JSON Schema's format support is a mess! ๐ซ
Observations
RAWmakes sense, and would be the default for any integer field property.IDmakes senseTIME_POINTvsTIME_SPANmake sense โ we see these probably often in DrupalCOUNTvsSEQUENCEmakes sense. I thinkSEQUENCEinvokes too many programmer associations, so I thinkORDINALorPOSITIONwould make more sense- I was surprised by
EXTENT. Dictionary:
extent
noun
1 the garden was about two acres in extent. area, size, expanse, length, stretch, range, scope, compass; proportions, dimensions.
2 she kept the full extent of her father's illness from her cousin. degree, scale, level, magnitude, scope, extensiveness, amount, size; coverage, breadth, width, reach, range, compass, comprehensiveness, thoroughness, completeness, all-inclusiveness.Kind of makes sense now โฆ except I don't quite know what we'd use these for. Which brings me to:
- The clear physical quantity measurements:
LENGTH,MASS,TEMPERATURE. The problem here is IMHO that it's such a particular subset that it makes it โฆ impossible (?) to judge whether it makes sense.Wikipedia to the rescue! https://en.wikipedia.org/wiki/International_System_of_Quantities defines 7 base phsyical quantities, from which all others can be derived. But how often are we dealing with "electric current" or "luminous intensity" in Drupal?! ๐ Seems overkill.
Following the rabbit hole: https://en.wikipedia.org/wiki/List_of_physical_quantities classifies these as scalar vs vector vs tensor physical quantities (TIL! ๐ฎ). There's also dimensionless ones: https://en.wikipedia.org/wiki/List_of_dimensionless_quantities.
https://en.wikipedia.org/wiki/Number#Main_classification doesn't help at all โ that's math, not semantics for numbers in the real world.
โจ Allow linking integer timestamps to `type: string, format: date` Active is forcing me to push this forward. Let's use AI to make this A) go faster, B) remove my biases.
Attempt 1: 149 different ones ๐
AI suggestion (by Kimi K2) created using this prompt:
Two integers might be describing wildly different things: just a number, a year, a timestamp, a count, a measure (weight in kg or pounds, length in cm or inches, distance, etc.), dimensions (width/height), a ratio โ you name it! Generate a throughful classification of the possible semantics, in the form of a PHP class with constants, and a comment for each constant describing that particular semantic.
<?php
/**
* Semantic categories that can hide behind a plain integer.
*
* The same numeric value (e.g. 2025) can mean:
* - a year
* - a timestamp in seconds since 1970-01-01
* - a product code
* - a count of items
* - a distance in kilometres
* - โฆ
*
* Use these constants as tags when you need to disambiguate the meaning of an
* integer that comes without context.
*/
final class IntegerSemantics
{
/* -----------------------------------------------------------------------
* 1. RAW / ABSTRACT
* --------------------------------------------------------------------- */
/** A pure mathematical integer without any physical interpretation. */
public const RAW_NUMBER = 1;
/** An identifier that is only meaningful inside a specific namespace. */
public const IDENTIFIER = 2;
/* -----------------------------------------------------------------------
* 2. TEMPORAL
* --------------------------------------------------------------------- */
/** Gregorian year (e.g. 2025). */
public const YEAR = 10;
/** Month of the year (1-12). */
public const MONTH = 11;
/** Day of the month (1-31). */
public const DAY_OF_MONTH = 12;
/** Day of the week (1-7 or 0-6 depending on locale). */
public const DAY_OF_WEEK = 13;
/** Hour of the day in 24-hour format (0-23). */
public const HOUR_24 = 14;
/** Minute (0-59). */
public const MINUTE = 15;
/** Second (0-59). */
public const SECOND = 16;
/** Unix timestamp โ seconds since 1970-01-01 00:00:00 UTC. */
public const TIMESTAMP_SECONDS = 17;
/** Milliseconds since 1970-01-01 00:00:00 UTC. */
public const TIMESTAMP_MILLIS = 18;
/** Microseconds since 1970-01-01 00:00:00 UTC. */
public const TIMESTAMP_MICROS = 19;
/* -----------------------------------------------------------------------
* 3. COUNT / CARDINALITY
* --------------------------------------------------------------------- */
/** Absolute number of discrete items (apples, users, bytes, โฆ). */
public const COUNT = 30;
/** Frequency โ how many times an event occurs per unit of time. */
public const FREQUENCY = 31;
/** Ordinal position โ 1st, 2nd, 3rd โฆ (human-facing, not zero-based). */
public const ORDINAL = 32;
/** Zero-based index as used in arrays and loops. */
public const INDEX_ZERO = 33;
/** Human-facing page number (starts at 1). */
public const PAGE_NUMBER = 34;
/* -----------------------------------------------------------------------
* 4. LINEAR MEASUREMENTS (SI & IMPERIAL)
* --------------------------------------------------------------------- */
/** Distance in millimetres. */
public const DISTANCE_MM = 40;
/** Distance in centimetres. */
public const DISTANCE_CM = 41;
/** Distance in metres. */
public const DISTANCE_M = 42;
/** Distance in kilometres. */
public const DISTANCE_KM = 43;
/** Distance in inches. */
public const DISTANCE_INCH = 44;
/** Distance in feet. */
public const DISTANCE_FOOT = 45;
/** Distance in miles. */
public const DISTANCE_MILE = 46;
/* -----------------------------------------------------------------------
* 5. AREA
* --------------------------------------------------------------------- */
/** Area in square metres. */
public const AREA_M2 = 50;
/** Area in square kilometres. */
public const AREA_KM2 = 51;
/** Area in hectares (1 ha = 10 000 mยฒ). */
public const AREA_HECTARE = 52;
/** Area in acres. */
public const AREA_ACRE = 53;
/* -----------------------------------------------------------------------
* 6. VOLUME
* --------------------------------------------------------------------- */
/** Volume in millilitres. */
public const VOLUME_ML = 60;
/** Volume in litres. */
public const VOLUME_L = 61;
/** Volume in cubic metres. */
public const VOLUME_M3 = 62;
/** Volume in US fluid ounces. */
public const VOLUME_US_FL_OZ = 63;
/** Volume in US gallons. */
public const VOLUME_US_GAL = 64;
/* -----------------------------------------------------------------------
* 7. MASS / WEIGHT
* --------------------------------------------------------------------- */
/** Mass in milligrams. */
public const MASS_MG = 70;
/** Mass in grams. */
public const MASS_G = 71;
/** Mass in kilograms. */
public const MASS_KG = 72;
/** Mass in metric tonnes (1000 kg). */
public const MASS_TONNE = 73;
/** Mass in ounces. */
public const MASS_OZ = 74;
/** Mass in pounds. */
public const MASS_LB = 75;
/* -----------------------------------------------------------------------
* 8. TEMPERATURE
* --------------------------------------------------------------------- */
/** Temperature in degrees Celsius. */
public const TEMP_CELSIUS = 80;
/** Temperature in degrees Fahrenheit. */
public const TEMP_FAHRENHEIT = 81;
/** Temperature in kelvin. */
public const TEMP_KELVIN = 82;
/* -----------------------------------------------------------------------
* 9. COMPUTING / DIGITAL
* --------------------------------------------------------------------- */
/** Memory size in bytes. */
public const BYTES = 90;
/** Memory size in kilobytes (1024 B). */
public const KILOBYTES = 91;
/** Memory size in megabytes (1024ยฒ B). */
public const MEGABYTES = 92;
/** Memory size in gigabytes (1024ยณ B). */
public const GIGABYTES = 93;
/** Bitmask / flags โ each bit carries independent Boolean meaning. */
public const BITMASK = 94;
/** RGB colour channel intensity (0-255). */
public const RGB_CHANNEL = 95;
/** HTTP status code (100-599). */
public const HTTP_STATUS = 96;
/** Exit code returned by a process (0-255). */
public const EXIT_CODE = 97;
/* -----------------------------------------------------------------------
* 10. FINANCIAL
* --------------------------------------------------------------------- */
/** Monetary amount in the smallest currency unit (cents, pence, โฆ). */
public const MONEY_CENTS = 100;
/** Basis points โ 1/100th of a percent (0-10 000). */
public const BASIS_POINTS = 101;
/** Currency multiplier โ how many units of currency A make currency B. */
public const EXCHANGE_RATE_SCALE = 102;
/* -----------------------------------------------------------------------
* 11. RATIOS / PROPORTIONS
* --------------------------------------------------------------------- */
/** Percentage (0-100). */
public const PERCENT = 110;
/** Permille (0-1000). */
public const PERMILLE = 111;
/** Parts per million. */
public const PPM = 112;
/** Aspect ratio numerator (width in width:height). */
public const ASPECT_WIDTH = 113;
/** Aspect ratio denominator (height in width:height). */
public const ASPECT_HEIGHT = 114;
/* -----------------------------------------------------------------------
* 12. DIMENSION PAIRS
* --------------------------------------------------------------------- */
/** Width in pixels on a screen. */
public const PIXEL_WIDTH = 120;
/** Height in pixels on a screen. */
public const PIXEL_HEIGHT = 121;
/** Width in centimetres for physical objects. */
public const PHYSICAL_WIDTH_CM = 122;
/** Height in centimetres for physical objects. */
public const PHYSICAL_HEIGHT_CM = 123;
/* -----------------------------------------------------------------------
* 13. ENERGY / POWER
* --------------------------------------------------------------------- */
/** Energy in joules. */
public const ENERGY_JOULE = 130;
/** Energy in kilowatt-hours. */
public const ENERGY_KWH = 131;
/** Power in watts. */
public const POWER_WATT = 132;
/** Power in kilowatts. */
public const POWER_KILOWATT = 133;
/* -----------------------------------------------------------------------
* 14. MISCELLANEOUS
* --------------------------------------------------------------------- */
/** Angle in degrees (0-360). */
public const ANGLE_DEGREE = 140;
/** Angle in radians (0-2ฯ). */
public const ANGLE_RADIAN = 141;
/** Speed in kilometres per hour. */
public const SPEED_KMH = 142;
/** Speed in miles per hour. */
public const SPEED_MPH = 143;
/** Pressure in pascals. */
public const PRESSURE_PASCAL = 144;
/** Pressure in hectopascals (1 hPa = 100 Pa). */
public const PRESSURE_HECTOPASCAL = 145;
/** Electric potential in volts. */
public const ELECTRIC_POTENTIAL_VOLT = 146;
/** Electric current in amperes. */
public const ELECTRIC_CURRENT_AMPERE = 147;
/** Electric resistance in ohms. */
public const RESISTANCE_OHM = 148;
/** Sound pressure level in decibels. */
public const SOUND_DB = 149;
}
Attempt 2: 146
Combine all that are expressing the same thing just using different units. For example: SPEED_KMH and SPEED_MPH should just be "speed". ORDINAL and PAGE_NUMBER are similarly equivalent. Combine all equivalents, and update corresponding comments to capture for human readers what each constant/semantic is intending to express.
โฆ yielded 146. It didn't quite listen.
Attempt 3: 12
That's still too granular. Summarize into at most a dozen, not >100!
<?php
/**
* What an integer can *mean* โ reduced to the essentials.
*
* Units are irrelevant: 60 could be โsecondsโ or โminutesโ and still be
* TIME_SPAN; 1000 could be โbytesโ or โgigabytesโ and still be DATA_SIZE.
*/
final class IntegerSemantics
{
/** Pure number, no physical meaning. */
public const RAW = 1;
/** Identifier or code that is only valid inside a given namespace. */
public const ID = 2;
/** Calendar year, month, day, hour, minute, second, Unix epoch, โฆ */
public const TIME_POINT = 3;
/** Duration, age, timeout, TTL, โฆ (any unit). */
public const TIME_SPAN = 4;
/** Count of items, events, bytes, pixels, people, โฆ */
public const COUNT = 5;
/** 0-based index, 1-based ordinal, page number, ranking, โฆ */
public const SEQUENCE = 6;
/** Length, width, height, depth, radius, distance, โฆ (any unit). */
public const LENGTH = 7;
/** Area or volume derived from length (any unit). */
public const EXTENT = 8;
/** Mass or weight (any unit). */
public const MASS = 9;
/** Temperature (any scale). */
public const TEMPERATURE = 10;
/** Memory, file size, bandwidth, โฆ (any unit). */
public const DATA_SIZE = 11;
/** Money in the smallest currency unit (cents, pence, โฆ). */
public const MONEY = 12;
}
Now we're talking!
Yes, please!
Are there more things you want to do on this MR, or is it ready for review?
โฆ but first, we need an MR.
Wow, excellent write-up!
Oversight in ๐ Text formatted with CKEditor within Canvas gets double escaped when output Needs work .
As soon as you convert that to an MR, I'll merge it! ๐
Why not a kernel test? See MR.
I see lots of progress here in the past ~24 hours by @larowlan & @penyaskito. ๐คฉ
Down to 4 failures! Just reviewed it. Looking good!
I got looped into 1.0 blockers shortly after #25. Now looking into #25 and โจ Allow linking integer timestamps to `type: string, format: date` Active for real.
I think many of these are legitimate.
Like the one that Scott just confirmed.
Why not just turn this into a bunch of -tagged issues? ๐ WDYT? :D
Previously reported: ๐ Media Library Widget Fails with BaseFieldDefinition Active . Has a lot more information. But @penyaskito investigated this one, so making this one win.
@penyaskito, please move over all issue credit. ๐
@mayur-sose: please check the issue queue next time to avoid duplicates. ๐
๐ Error 500 on Canvas After Deleting Media Type "Image" Active is a duplicate, but actually tracked this down to a core bug. I'll make sure you're all credited for your contributions!
o me, this is a textbook example of an appropriate case for a "just-in-time" update.
I forgot about that!
I even wrote that in #2, but that was 1.5 month ago. So I forgot! Mea culpa ๐
Let's do this. ๐
Please populate "related issues" in the future. ๐
Very impressive MR! ๐
Missing a follow-up to add support for indexing blocks.
A number of things are unclear on the MR โ ranging from "ignored prop names" AFAICT being too coarse, to some dead code, and more.
MR up, feel free to merge, then!
Looks like there's a bug in there: ๐ Exported Canvas pages with media references have incorrect syntax Active .
This was introduced in โจ Add ComponentSourceInterface::getReferencedEntities Active , for an 11.3-only feature. It's got test coverage, but this suggests something was missed.
- Paired with @f.mazeikis because he got stuck on expanding what I did in #17 on how to also support image/video prop shapes, i.e. how to support object shapes in addition to only scalar shapes. Unblocked now.
- ๐ This surfaced that what
๐
`::matchEntityPropsForObject()` is too naรฏve
Active
did for
DynamicPropSources, this must do forStaticPropSources: first follow references as deep as possible and THEN evaluate into an object shape (FieldObjectPropExpression.
Doing that is needed to be able to reuse the previously introducedReferencedBundleSpecificBranches. Which in turn makes things MUCH simpler forhook_canvas_storable_prop_shape_alter()implementations. So we jointly concluded that the "multi-bundle-but-same-shape" support that I introduced in ๐ Decouple image shape matching from the `image` MediaType Active should be deprecated. - Reviewed the MR and left lots of concrete pointers.
- Issue summary still needs to be updated.
- Placeholder CR created: https://www.drupal.org/node/3563451 โ โ used for the deprecation.
Oops, forgot to push this local commit:
src/Plugin/DataType/ComponentInputs.php | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/src/Plugin/DataType/ComponentInputs.php b/src/Plugin/DataType/ComponentInputs.php
index c82a72cf9..d4d5e35ec 100644
--- a/src/Plugin/DataType/ComponentInputs.php
+++ b/src/Plugin/DataType/ComponentInputs.php
@@ -35,6 +35,14 @@ use Drupal\canvas\PropSource\StaticPropSource;
id: "component_inputs",
label: new TranslatableMarkup("Component inputs"),
description: new TranslatableMarkup("The input values for the components in a component tree: without structure"),
+ // TRICKY: this does not provide validation constraints, because this is
+ // validated at the component instance level. Component (instance) inputs can
+ // only be validated by the ComponentSource plugin providing this component
+ // that powers this component instance (and is referenced using the Component
+ // ID and version).
+ // @see \Drupal\canvas\ComponentSource\ComponentSourceInterface::validateComponentInput()
+ // @see \Drupal\canvas\Plugin\Field\FieldType\ComponentTreeItem
+ // @see \Drupal\canvas\Plugin\Validation\Constraint\ValidComponentTreeItemConstraintValidator
)]
final class ComponentInputs extends TypedData implements ContentAwareDependentInterface {
Do you think that's worth adding, @phenaproxima?
I told @phenaproxima that what he described to me during our brief call when discussing this issue, reminded me of ๐ Config validation: config entities should get the same validation errors when validated as plain config vs ConfigEntityAdapter Needs review โ that seems to be the most plausible explanation, especially together with the observation that all originally bugs reported here turned out to be actually working as expected.
This spawned one more issue: ๐ The `ComponentTreeMeetRequirements` is no longer accurately named Active .
Unfortunately, every single โinputs are not validatedโ comment you wrote was easily disproven, so I included a screenshot for each. Because I wanted to check if any of them were indeed true, which would indeed have been a major red flag. Which means โฆ I reverted 99% of what this MR did ๐๐ฌ
It's hard to pick.
- Is better because it requires fewest changes
- Is better because it's more coherent: it ensures that the requirements are consistent over the entire component tree, but it will require some (simple) refactoring of the current constraint
I'll let @phenaproxima pick.
wim leers โ created an issue.
I set out to reproduce the first example in #7:
This is creating a component_tree field item list with no constraints on it apart from the one in
\Drupal\canvas\Plugin\Field\FieldType\ComponentTreeItemList::getConstraints(), which isComponentTreeStructureConstraint. So the structure is validated but not the data.
โฆ and failed. ๐
โ You probably looked at the "empty component tree" test case โ where that is indeed the only validation constraint. But for any of the non-empty test cases for that test method, all the other validation constraints are being executed:
Just paired with @phenaproxima to show him this finding.
This made @phenaproxima realize the problem actually lies elsewhere. He's working to bring clarity to what that is exactly.
So, we decided to transform this issue to a docs-only MR, to improve the status quo ๐
#7++
Very nice!
Digging in.
P.S.: can you please update the IS to explain how this connects to ๐ Remove the ability for component instances to deviate from the field type + storage + instance settings dictated by the `Component` version Active ? (You told me in Acquia Slack this was spawned from there.)
For a module as validation-heavy as Canvas is, there is one extremely puzzling omission:
\Drupal\canvas\Plugin\Field\FieldType\ComponentTreeItemListInstantiatorTrait::staticallyCreateDanglingComponentTreeItemList().It creates a
component_treeitem list, but fails to attach any validation constraints to the items in that list.
100% of them get validation constraints โฆ just not there, because that's pure "object instantiation infrastructure".
Config entities do not use these at load or save time, but do use them during validation and when rendering. It's what allows the same exact validation constraints (and render infrastructure) to be used for component trees in both content entities and config entities.
See:
- All config entities containing a component tree use
type: canvas.component_treeincanvas.schema.yml, which contains:
โฆ constraints: ComponentTreeStructure: [] ComponentTreeMeetRequirements: # By default, only StaticPropSources may be used in config-defined component trees. inputs: absence: # DynamicPropSources retrieve structured data from a content entity of a specific entity type and bundle. This # is typically the content entity that contains the component tree, but could also be a dynamically loaded # content entity. A config entity typically is not associated with any content entity, so require its absence # by default. - dynamic - host-entity-url # Not yet supported by the Canvas UI. - adapter presence: ~ tree: absence: ~ presence: ~ FullyValidatable: ~ - All content entities containing a component tree use
\Drupal\canvas\Plugin\Field\FieldType\ComponentTreeItem, which contains:
โฆ constraints: [ 'ValidComponentTreeItem' => [], 'ComponentTreeMeetRequirements' => [ // Only StaticPropSources may be used, because using DynamicPropSources or // HostEntityUrlPropSources a decision that should be made at the Content // Template level by a Site Builder, not by each Content Creator. // @see https://www.drupal.org/project/canvas/issues/3455629 'inputs' => [ 'absence' => [ 'dynamic', 'host-entity-url', // @todo Allow adapters that consume a single shape and output that same single shape in https://www.drupal.org/project/canvas/issues/3536115 'adapter', ], 'presence' => NULL, ], 'tree' => [ 'absence' => [ // Components implementing either of these 2 interfaces are only // allowed to live at the PageRegion level. // @see \Drupal\canvas\Entity\PageRegion // @see `type: canvas.page_region.*` TitleBlockPluginInterface::class, MessagesBlockPluginInterface::class, ], 'presence' => NULL, ], ], ], โฆBut if you call
$dangling_component_tree->validate()directly for any reason -- the tests do this quite a bit! -- no validation happens at all. Because the thing has no constraints on it.Oh! Where? That'd indeed be a problem!
So: more than willing to change/improve things! Can you point to concrete cases where this is causing a problem? Better yet, can you create an MR that makes a few tests fail? :)
If this is ever reopened, this needs STR.
This was created October 30. RCs started on October 11: https://www.drupal.org/project/canvas/releases/1.0.0-rc1 โ
โ this will need update paths and test coverage
(i.e. ContentTemplate config entities need to be updated)
Not appreciated.undoing these poor testing decisions should be prioritized.
Time pressure on the Canvas team has been insane. The choice we had was very often:
- either the local optimum of "meet the deadline and have any testing at all"
- or the even worse "make the deadline without any tests"
If it were up to others, we'd have picked #2 easily a hundred times. I insisted on #1.
Which means now we get to refactor a painful set of tests. Otherwise, we'd have to still write them from scratch.
Which means we now get to refactor tests with medium-to-high confidence in the worst case. Otherwise, it'd have been write tests with low-to-medium confidence in the best case.
Let's chip away at making things better, which is always the case in software development ๐
As you know, entity references in Drupal can be, by design, stale.
Indeed.
And I'm saying that precisely that is what was missing. Specifically:
Wouldn't that be a problem in the case the referenced entity was temporarily deleted and then recreated? Then the absence of the referenced entity's cache tag would mean the rendered result would remain stale!
is the problem that https://git.drupalcode.org/project/canvas/-/merge_requests/300/diffs?com... aimed to fix:
But I cannot really follow what the proposed middle-ground is trying to do, could you clarify?
If the referenced entity fails to load because it is missing, that might be caused either by an actual deletion (which would indeed refer to a stale reference, for which
#1368386: Delete references to deleted entities โ
is the core issue to fix), or by a temporary state that could occur e.g. while deploying code/executing scripts/applying a recipe (which should happen for recipe applying since #3442022 but given its ancestry, is likely to be incomplete, see
#3442022-17: Trigger entity validation in \Drupal\Core\DefaultContent\Importer::importContent() โ
).
This could for example occur when the referenced entity is not yet saved, because \Drupal\Core\Entity\Plugin\Validation\Constraint\ValidReferenceConstraintValidator also allows referencing new (unsaved) entities.
So yes, this is me trying to think about all edge cases and be extra cautious in Canvas since it depends on a super complex tree of dependencies. I've been bitten too many times by the content, config, migration and recipe systems not performing the validation one would reasonably expect ๐จ
wim leers โ changed the visibility of the branch 1.x to hidden.
11.3 is about to ship, but Canvas won't start requiring 11.3 in the foreseeable future โ probably not until 2.x.
Nice catch on https://git.drupalcode.org/project/drupal/-/merge_requests/14034/diffs?c... โ I definitely didn't think about the scenario where an early 11.3 RC adopter would have already modified the config! ๐
โจ [upstream] Use CKEditor 5's native and UX Needs work was fixed upstream!
Which AFAICT means that actually this should now work without any changes to Canvas: as long as Canvas continues to support 11.2.x as the minimum version, its default config should remain unchanged.
Would be great if @phenaproxima could confirm.
@penyaskito is more familiar with Tugboat than I am, hoping he can review :)
Added ๐ Remove usages of code deprecated in symfony 7.4 Active .
Thanks for pushing this forward!
There's a few bits that need work.
Oh, I think this is simply a matter of having created the MR from the wrong branch? https://git.drupalcode.org/issue/canvas-3561270/-/compare/1.x...sujal-us... needs to get an MR, not the 1.x branch that's been pushed up!
MR 412 contains 1 commit and seems to delete and remove every line in the codebase ๐ Something went quite wrong here.
or better, reference
canvas.api.phpto avoid duplication.
+1
For ๐ (and response to #7)
#13++ โ agreed with everything you wrote (why this will automatically get test coverage, the err-on-the-side-of-caution thing etc).
P.S.: linking the issue @penyaskito determined introduced the caution.
Yay! Lovely progress!
Linked all related issues to improve discoverability.
Ready for final sign-off. Followed through on all next steps I listed in #44, and added much-needed docs/diagrams :)
P.S.: do we also want to update /API.md? I'd be fine leaving that for a follow-up, to allow that to be its own conscious decision.
- Ran into obstacles while getting this green: https://github.com/phpstan/phpstan/issues/13566#issuecomment-3645405380
- Created a diagram for how all this works, because if we want to open up this API, then I think having a diagram to make this more widely understood without having to read code is required for sanity. Sadly, GitLab's lagging in its Mermaid version (10, 11 has been out for over a year), so it's best viewed on mermaid.live โ until GitLab updates, that is!
Back to @penyaskito for final review, because I made too many changes to be able to just go ahead and merge.
The key things I did:
- Tests weren't passing due to a small bug in the excellent new test coverage @penyaskito wrote.
- simplified
- Quite a significant refactor of
(ReadOnly)PropShapeRepository, to(Persistent|Ephemeral)PropShapeRepository. See details+reasoning+walkthrough.
Remaining for @penyaskito:
- review that refactor
- update
SingleDirectoryComponentTest::testDiscovery()expectations - https://git.drupalcode.org/project/canvas/-/merge_requests/145#note_634958 needs to be answered
- https://git.drupalcode.org/project/canvas/-/merge_requests/145#note_638367 needs to be addressed, and in doing so should make https://git.drupalcode.org/project/canvas/-/merge_requests/145#note_638364 disappear
- restore
canvas.api.php: https://git.drupalcode.org/project/canvas/-/merge_requests/145#note_638555 - address https://git.drupalcode.org/project/canvas/-/merge_requests/145#note_638497
I think 2 other MRs should land first, to ensure we don't introduce a regression in the form of an unwarranted and unwanted broadening here โ see https://git.drupalcode.org/project/canvas/-/merge_requests/318#note_637892 for details.
@jessebaker provided the pointer I needed, and it was a silly oversight on my part!
Over to @jessebaker for the finishing touch. Already approved :)
All back-end CI jobs are green. Playwright tests AFAICT need ARIA snapshot updates (which @jessebaker knows to update faster than me). The CLI test CI job points at a back-end issue, but I suspect there's a bug in the test: I suspect there's something off about the request it sends.
I can't get that test to run locally, and the architect of it (@balintbrews) is stumped too, so rather than spending more time on it, I asked @jessebaker to extract that request body so I can reproduce that bit and fix it.
@jessebaker asked me to help out with that last obstacle: the previews of code components. Rather than fighting their omission, I instead worked to make them just work:
๐ They even work for auto-saved changes! Which is IMHO a major usability improvement, because if multiple front-end developers are collaborating on code components, the preview-upon-hover now shows an accurate preview of the current auto-saved state!
wim leers โ made their first commit to this issueโs fork.
๐ Refactor, improve UX of side panels and introduce component management Active is tackling most of this issue's scope, so this issue should be postponed on that one.
Synced with @f.mazeikis this morning. He got stuck on the "string and object representations" foundations that this issue needs (which are non-trivial to wrap your head around!).
I've set the majority of those foundations up. PropExpressionTest has pretty detailed unit test coverage. One @todo is left in \Drupal\canvas\PropExpressions\StructuredData\ReferencedBundleSpecificBranches::__construct().
Beyond the foundations, I've also added \Drupal\canvas\PropExpressions\StructuredData\ReferenceFieldTypePropExpression::withAdditionalBranch() and ::withoutBranch(), for hook_storage_prop_shape_alter() implementations to keep things simple.
Next up:
PropExpressionKernelTestis failing because calculating dependencies is not yet being passed through; is trivial- updating the
Evaluator(@f.mazeikis already already had code for this on his local branch) - test coverage for that
- The aforementioned
@todo - Create the follow-up issue to add branching support to
ReferenceFieldPropExpression(necessary forDynamicPropSourcematching, but that will in turn also require logic updates to the shape matching logic). This means we can also defer updating theLabelerto that follow-up.
@f.mazeikis: I'm available in the coming days to help you keep moving. If you want reviews, assign this issue to me ๐