Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
Account created on 26 December 2006, about 19 years ago
  • Senior Principal Software Engineer โ€” Drupal Acceleration Team at Acquiaย  โ€ฆ
#

Merge Requests

More

Recent comments

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

wim leers โ†’ created an issue.

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

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! ๐Ÿ‘‹

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

#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:

  1. Make AdaptedPropSource itself throw an exception in non-test contexts when using any adapter other than \Drupal\canvas\Plugin\Adapter\UnixTimestampToDateAdapter
  2. Introduce a new AdaptedDynamicPropSource, which only accepts single-input adapters (or even just the UNIX-to-date adapter)
  3. Introduce a new feature on DynamicPropSource itself, 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.

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

I defer to our Code Clarity Chief, @phenaproxima.

So: consider this a pre-emptive RTBC ๐Ÿ˜Š

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

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!

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

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?

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

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.

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

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!

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

Retitling to comply with our Code Clarity Chief's preferences ๐Ÿค“ /me waves to @phenaproxima

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

๐Ÿ“Œ 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! ๐ŸŽ‰

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

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

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

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! ๐Ÿ‘

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

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.

There are 2 parts to achieving this:
  1. integer (and number) semantics: ๐Ÿ“Œ Add `IntegerSemanticsConstraint` Active , because otherwise even if we have adapters, we'll get nonsensical suggestions when using the Canvas ContentTemplate UI
  2. 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!) AdaptedPropSource to end up being stored in our various component trees
  3. Nope, not actually a problem, because โœจ Real-time preview for props changes of JS components Active applies only to StaticPropSources, not linked DynamicPropSources! ๐ŸŽ‰

Of these:

  1. 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 โ†’
  2. Is to avoid disruption/risk โ†’ expect MR tomorrow
๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

I prefer approach #2, but I'd love to hear @phenaproxima's take ๐Ÿ˜Š

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

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 .

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

Wow! I just ran into this while working on ๐Ÿ“Œ Add `IntegerSemanticsConstraint` Active ! So nice to see this fixed already ๐Ÿคฉ

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

Retitling to reflect the actual purpose.

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

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:

  1. raw
  2. ID
  3. count
  4. position
  5. 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! ๐Ÿซ 

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

Observations

  • RAW makes sense, and would be the default for any integer field property.
  • ID makes sense
  • TIME_POINT vs TIME_SPAN make sense โ€” we see these probably often in Drupal
  • COUNT vs SEQUENCE makes sense. I think SEQUENCE invokes too many programmer associations, so I think ORDINAL or POSITION would 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.

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

โœจ 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!

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

Yes, please!

Are there more things you want to do on this MR, or is it ready for review?

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

+1 to all that โ€” thanks!

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

โ€ฆ but first, we need an MR.

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

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! ๐Ÿ˜Š

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

Why not a kernel test? See MR.

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

I see lots of progress here in the past ~24 hours by @larowlan & @penyaskito. ๐Ÿคฉ

Down to 4 failures! Just reviewed it. Looking good!

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

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.

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

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

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

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. ๐Ÿ™

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

๐Ÿ› 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!

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

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. ๐Ÿ˜Š

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

Please populate "related issues" in the future. ๐Ÿ™

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

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.

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

MR up, feel free to merge, then!

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

Looks like there's a bug in there: ๐Ÿ› Exported Canvas pages with media references have incorrect syntax Active .

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

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.

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
  • 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 for StaticPropSources: 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 introduced ReferencedBundleSpecificBranches. Which in turn makes things MUCH simpler for hook_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.
๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

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?

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

Bump, let's get this done?

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

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.

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

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 ๐Ÿ™ˆ๐Ÿ˜ฌ

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

It's hard to pick.

  1. Is better because it requires fewest changes
  2. 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.

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

wim leers โ†’ created an issue.

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

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 is ComponentTreeStructureConstraint. 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 ๐Ÿ‘

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

#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.)

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

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_tree item 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_tree in canvas.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? :)

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

If this is ever reopened, this needs STR.

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

Thanks!

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

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)

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

undoing these poor testing decisions should be prioritized.

Not appreciated.

Time pressure on the Canvas team has been insane. The choice we had was very often:

  1. either the local optimum of "meet the deadline and have any testing at all"
  2. 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 ๐Ÿ˜Š

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

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 ๐Ÿ˜จ

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

wim leers โ†’ changed the visibility of the branch 1.x to hidden.

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

11.3 is about to ship, but Canvas won't start requiring 11.3 in the foreseeable future โ€” probably not until 2.x.

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

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! ๐Ÿ™ˆ

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

โœจ [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.

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

Thanks!

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

@penyaskito is more familiar with Tugboat than I am, hoping he can review :)

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

LGTM, thanks!

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

Thanks for pushing this forward!

There's a few bits that need work.

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

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!

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

MR 412 contains 1 commit and seems to delete and remove every line in the codebase ๐Ÿ˜… Something went quite wrong here.

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

or better, reference canvas.api.php to avoid duplication.

+1

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

For ๐Ÿ˜… (and response to #7)

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

#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.

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

Yay! Lovely progress!

Linked all related issues to improve discoverability.

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

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.

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
  1. Ran into obstacles while getting this green: https://github.com/phpstan/phpstan/issues/13566#issuecomment-3645405380
  2. 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!
๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

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:

  1. Tests weren't passing due to a small bug in the excellent new test coverage @penyaskito wrote.
  2. simplified
  3. Quite a significant refactor of (ReadOnly)PropShapeRepository, to (Persistent|Ephemeral)PropShapeRepository. See details+reasoning+walkthrough.

Remaining for @penyaskito:

  1. review that refactor
  2. update SingleDirectoryComponentTest::testDiscovery() expectations
  3. https://git.drupalcode.org/project/canvas/-/merge_requests/145#note_634958 needs to be answered
  4. 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
  5. restore canvas.api.php: https://git.drupalcode.org/project/canvas/-/merge_requests/145#note_638555
  6. address https://git.drupalcode.org/project/canvas/-/merge_requests/145#note_638497
๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

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.

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

@jessebaker provided the pointer I needed, and it was a silly oversight on my part!

Over to @jessebaker for the finishing touch. Already approved :)

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

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.

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

@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!

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

wim leers โ†’ made their first commit to this issueโ€™s fork.

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

๐Ÿ“Œ 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.

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

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:

  • PropExpressionKernelTest is 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 for DynamicPropSource matching, but that will in turn also require logic updates to the shape matching logic). This means we can also defer updating the Labeler to 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 ๐Ÿ™

Production build 0.71.5 2024