MR as POC of #6, still needs additional test coverage.
denormalizeFromClientSide() doesn't have to be static, instead we can convert it to an instance method that updates the instance directly.
longwave → made their first commit to this issue’s fork.
This feels like a job for a contrib module and not something we should encourage in core.
More fundamental question: should we reset the validator before each time we try to use it, instead of after?
As it's a dev dependency we could consider bumping the lockfile version to 6.x in 11.2 (while still allowing downgrades to 5.x), similar to us upgrading from PHPStan 1 to 2.
Committed baa7ad0 and pushed to 11.x. Thanks!
Not eligible for backport as registerCacheTagsForPreload() does not exist in previous versions.
I had a hunch that
📌
Split model values into resolved and raw
Active
would largely fix this, and it seems correct; tested with xb_demo there is a 230x speed improvement in ::clientModelToInput() which is by far the largest single section of the flamegraph.
Found a bug when testing this with xb-demo, see MR.
However the server side performance issues with xb-demo are just gone with this patch, the 70 calls to GeneratedFieldExplicitInputUxComponentSourceBase::getClientSideInfo() now take 92ms down from 21,158ms - a 230x speed improvement :D
Feedback addressed, added another followup as per discussion, back to RTBC.
Waiting on this to land before I try to move the other image issues forward because this is intertwined with some of them.
longwave → created an issue.
Added some nits but as usual this looks good, leaving at NR so Wim or others can still review.
This is won't fix if we do 📌 Vet require-dev dependency justinrainbow/json-schema as a require dependency Active instead.
#7 would be solved by 📌 Move JSON:API validation to a feature flag or separate module Active
Made a start on this.
longwave → created an issue.
I looked at this for a while but I don't think we need additional test coverage here, the changes in 📌 Split model values into resolved and raw Active mean that this issue should go away, suggest closing this as outdated or duplicate.
longwave → made their first commit to this issue’s fork.
This is just reverting the previous workaround, which turned out to be a CI issue that we have now solved, so let's just clean this up.
One nit with test formatting but otherwise no notes, this looks great - good find on that stored data vs later shape change bug, thanks also for the detailed self-review.
Rebased and added assertions to media-library.cy.js to demonstrate the issue. The canvas starts with a sample image inserted, which we assert correctly, then the Remove button is clicked, which (I believe) should fall back to the example image for the component - but this doesn't happen at the moment so the test fails.
longwave → made their first commit to this issue’s fork.
We already have the composer drupal-phpunit-upgrade command that should still work if the constraints are set correctly back from when we supported both PHPUnit 8 and 9.
Backported down to 10.4.x as an eligible bug fix and contrib blocker.
Committed and pushed 72658312419 to 11.x and 7365716bb37 to 11.1.x and 6bb7ed74434 to 10.5.x and 414748a59c7 to 10.4.x. Thanks!
larowlan → credited longwave → .
Been thinking about this on and off for a while now. I agree that we need the raw values available to the client side UI, because in the case of
[
'alt' => 'This is a random image.',
'width' => 100,
'height' => 100,
'target_id' => (int) $this->mediaEntity->id(),
]
we need to send and receive the target_id for the media library form. This is the root cause of 🐛 Error when adding a section to the page Active - see Felix's investigation in #7.
Do we only need resolved values for component preview and final render? I have concerns that if we are storing/sending both raw and resolved then there is a chance of them getting out of sync - either accidentally, or deliberately if someone is trying to mess with the values, which might have security implications.
So in the layout API we would only send the layout + raw values to the client, the client stores and sends raw values back for forms (which is all we need anyway) and previews (which we translate at preview time into the resolved values). If this is correct, where do we need resolved values in the client side model?
quietone → credited longwave → .
Rebased.
I agree with everything stated here. Given that we now support two majors at once there is no need to jump to the latest release of Drupal immediately, but if you are starting a new project then you can and should be starting on both the newest Drupal and PHP in order to give yourself the maximum runway before having to upgrade. In order to keep core moving more quickly it is better that we don't have to support older PHP versions for an extended period of time.
Removing the RM tag as all RMs have posted comments in support.
Well, that was easy!
WordPress are discussing how this can be improved, and have even referenced Drupal and our iframe security settings: https://github.com/WordPress/performance/issues/1626
They seem to be running into the same issues that we have already discovered, and there is no solution yet other than the alternate domain.
#2831944-131: Implement media source plugin for remote video via oEmbed →
already investigated this a long time ago and found that we would have to add sandbox="allow-same-origin allow-scripts" to get YouTube and Vimeo running, which defeats the point of using the sandbox in the first place.
longwave → made their first commit to this issue’s fork.
From a release manager point of view #33 feels like it is going to cause us pain further down the line - we forever have to support the field tables that may or may not exist. I would prefer that we take this in more definite steps, and figure out how to hive off these fields to a separate module that can be moved to contrib.
Simply removing them from the form (as per the original intent of this issue) feels like an easier win and the most straightforward next step.
This does feel a bit heavy handed because the vast majority of GET requests do not require this as far as I can tell, so we're making a lot of busy work.
Can we detect writes to config or content entities via events or hooks, and if they were triggered by an HTTP GET request, ensure that it was protected via CSRF? If there is a case where this can happen where CSRF is not required, maybe we can just ensure the route has the CSRF check flag specified, even if it's set to false?
Ignore me, we self-host Sentry and the dashboards for this weren't showing up due to a misconfiguration on our side.
Yeah I wasn't quite sure how we would go about this.. I think it would require overriding the queue backend or queue factory for the producer side, and the same or perhaps Cron::processQueues() for the consumer part.
longwave → created an issue.
longwave → created an issue.
greggles → credited longwave → .
I tried to commit this but it's breaking the pre-commit script:
Running PHPStan on changed files.
------ -----------------------------------------------------------------------
Line
------ -----------------------------------------------------------------------
408 Syntax error, unexpected '"' on line 408
408 Syntax error, unexpected T_STRING on line 408
409 Syntax error, unexpected T_NS_SEPARATOR on line 409
409 Syntax error, unexpected T_STRING on line 409
426 Syntax error, unexpected T_LNUMBER on line 426
450 Syntax error, unexpected T_STRING on line 450
451 Syntax error, unexpected T_STRING on line 451
479 Syntax error, unexpected T_STRING on line 479
480 Syntax error, unexpected T_STRING on line 480
514 Syntax error, unexpected '.' on line 514
555 Syntax error, unexpected T_CONSTANT_ENCAPSED_STRING on line 555
555 Syntax error, unexpected T_STRING on line 555
556 Syntax error, unexpected T_CONSTANT_ENCAPSED_STRING on line 556
556 Syntax error, unexpected T_STRING on line 556
557 Syntax error, unexpected '"', expecting '-' or T_STRING or T_VARIABLE
or T_NUM_STRING on line 557
557 Syntax error, unexpected T_STRING, expecting T_VARIABLE or
T_ENCAPSED_AND_WHITESPACE or T_DOLLAR_OPEN_CURLY_BRACES or
T_CURLY_OPEN on line 557
I think because line 406 has the PHP open tag:
echo "<?php return [];" > ./core/.phpstan-baseline.php
then PHPStan tries to interpret the YAML as PHP and it all goes wrong. Either we need to tell PHPStan to ignore this file, or remove the open tag, or break it up into two parts so PHPStan can't see it?
https://www.drupal.org/project/drupal/releases/10.4.4 → has been tagged and will be available in the next few minutes, including this fix.
@bhanu951 cleared this issue with me before posting.
While we are here we should yarn audit all dependencies on all active branches as a cursory check shows that other transient JS dependencies have known vulnerabilities too.
While this is a bug fix it's also a minor behaviour change as mentioned in the IS, so only committing this to 11.x to avoid breaking existing sites in a patch release. The MR also doesn't apply cleanly to 10.5.x so only committing this to 11.x; if you think this is worthwhile to backport please reopen with an MR against 10.5.x.
Committed ee78cfa and pushed to 11.x. Thanks!
So if I understand the MR correctly this changes doGetUploadLocation() so it now always includes a trailing slash.
Unfortunately this is a public API change:
public function getUploadLocation($data = []) {
return static::doGetUploadLocation($this->getSettings(), $data);
}
Existing callers might not expect the trailing slash, so this could break other usages of this method in contrib or custom code?
Is there another way of fixing this without changing this method's return value?
Some nits about comments.
Added some questions/comments.
See comment on the MR.
+1 for this. Dependabot and similar tools already flag uninstalled modules because they only look at composer.json.
Also, this would help if there was ever another SA-CONTRIB-2016-039 → .
Huge improvement from a tiny change!
Committed and pushed to 11.x, thanks!
Welcome to the team!
Committed and pushed 3200a2ef9b5 to 11.x and 20d6ed8962e to 11.1.x and c93179c63dd to 10.5.x and 2a2b4a415a6 to 10.4.x. Thanks!
I've seen the same level of heavy writes here, this sounds like a good solution. I have a slight concern that we haven't covered all possible consequences but let's land this in 11.x - if we start seeing odd cache coherency issues then we can always back this out again.
Committed bb1f2a6 and pushed to 11.x. Thanks!
I read the MR and did a brief manual test and can't see any changes that are required.
Committed ea29660 and pushed to 11.x. Thanks!
Also published the change record.
#3169212: Improve transliteration of Ukrainian letters → also has some discussion on this.
I think this is a duplicate of 🐛 The placement of the component preview is sometimes inconsistent Active .
The bot gets confused when things don't apply to 11.x.
No more reports in two years, closing as outdated.
longwave → created an issue.
Wondering if this it the right way at all, maybe we should be altering the SDC component info much earlier to remap the example/default path instead of trying to map it in via a prop source.
Added a test for this and fixed it to only apply to object shapes; this works but it feels wrong? Also I don't like the way the fallbacks are checked both before and then after the inputs are set up, but too tired now to figure out how to refactor it nicely. The test passes locally though!
wim leers → credited longwave → .
This seems related to Wim's analysis in #3507929-25: Allow adding "image" props in the code component editor → .. not sure the fix is quite right but it's in the right location, will try to look later on today.
wim leers → credited longwave → .
@hooroomoo I've manually tested this and can't find any issues.. I can reproduce the delay on 0.x but with the MR in place it's gone, the Published > Changed label updates almost instantly.
wim leers → credited longwave → .
@smustgrave it means they don't have one of the more specific roles such as release manager, framework manager or product manager.
@joachim reminded me that this solution could also be used to solve 🐛 Wrong DRUPAL_ROOT with non-standard code structure Needs review
longwave → made their first commit to this issue’s fork.
Not sure what else we have to do here, maybe @poker10 and @mcdruid should post in this issue to confirm their acceptance?
In fact it was easy to resolve (didn't need resolving at all!) so let's just land this.
Committed 616d104 and pushed to 11.x. Thanks!
Merge conflict in composer.lock.
This is valid, the message is no longer optional. drupal_set_message() with no arguments returned the static set of messages, now that is handled by a separate method in the messenger service.
Backported down to 10.4.x as an eligible documentation fix.
Committed and pushed 4899db93d99 to 11.x and 8877da3b01b to 11.1.x and 46cf264bf32 to 10.5.x and 957c16fc138 to 10.4.x. Thanks!
Amazing that we can still eke out a few more seconds with minor tweaks.
Seems like a reasonable feature request, the amount of code added is tiny and I guess at least a few people want this somewhere.
Committed c8fcd85 and pushed to 11.x. Thanks!
This does not fit the criteria for maintenance minors so has not been backported to 10.x.
This is a nice self contained fix, backported down to 10.4.x as an eligible bug fix.
Committed and pushed 1ad501aafe7 to 11.x and 1a8a6184101 to 11.1.x and 766f32639c9 to 10.5.x and 09158e668be to 10.4.x. Thanks!
Added a suggestion to improve readability and also this will only land in 11.2 so I updated the deprecation notice.
Committed feff767 and pushed to 11.x. Thanks!
Also published the change record.
That should be the last of the test failures.
+1 for #9, we already use forbiddenAnnotations to prevent a mis-application of @inheritdoc so we can do the same for this.
Looking at test failures.
I think this is OK to land as highly experimental and have added even more comments to denote this. I am not sure that mapping each block plugin that might want to become a JS component is sustainable; I have concerns about both discoverability for developers and extensibility by contrib if we do continue with this solution. But, I don't have a better idea right now, and this unblocks the feature to see if it is at least workable and understandable by XB users who are experimenting with JS components.
The proposal now just appears to be rehashing the discussion in 🌱 [policy] Decide how long major Drupal versions should be supported Needs review . Note that the Drupal 10 EOL date is not yet set in stone:
We will try to support Drupal 10 until 12.0 is released, with a definite maximum EOL of Symfony 6's EOL. The final EOL date is TBD and to be reviewed closer to the time based on how things are going.
We may extend 10.5 further (to December 2026 even with a June 12.0, or into 2027 if that seems feasible and necessary), but we make that determination during Drupal 12 preparation and/or after its release.
These statements still stand; we may decide to extend Drupal 10 EOL, but we are not in a situation to make a decision on that yet, let alone the Drupal 12 EOL as per the issue summary here.
Drupal9 is a remnant from
#3254149: Remove config.autoloader-suffix from composer.json →
- but yeah if it can be overridden and isn't guaranteed to change then it's not reliable here I guess.
The class is only used for initialization so there is nothing holding on to it after the autoloader is set up but you can find the name
> array_filter(get_declared_classes(), fn($class) => str_starts_with($class, 'ComposerAutoloaderInit'));
= [
257 => "ComposerAutoloaderInit7f3c203e5297306f9277da08961993f2",
]
This is probably faster than serialize and hash?
Can we discover the class name of the autoloader, as that appears to contain the lockfile hash? This is presumably to work around similar problems with opcode caching.
"content-hash": "7f3c203e5297306f9277da08961993f2",
then in vendor/composer/autoload_real.php:
class ComposerAutoloaderInit7f3c203e5297306f9277da08961993f2
It doesn't prove anything unless you have spoken to Symfony and confirmed that they are willing and able to do this for other versions. Even then you still have the problem that both Symfony and Drupal will require additional funding to do this, for which I have seen no answers in this issue (or elsewhere, for that matter).
ddev manages this without a setting, I don't know how - I assume either an environment variable or something in the settings.php include - but if that's possible I don't see why we need this feature.
Also, how will this work on sites that listen on multiple domain names?
longwave → made their first commit to this issue’s fork.
+1 this change and the change record both look good to me
Picking this up as I've been solving other image issues in 📌 ComponentMetadataRequirementsChecker::check() should validate that the example(s) actually match the JSON schema Active
Assigning to Wim for review.
Since I moved the check to earlier then it seems the filtering is no longer needed, only the explicit continue, plus the fix to allow missing examples.
Updated the IS and migrated the original issue to 📌 ComponentMetadataRequirementsChecker::check() should validate that the example(s) actually match the JSON schema Active
longwave → created an issue.
Either value are not always present in prop source arrays, in which case we need to fix
Line src/PropSource/StaticPropSource.php
------ -----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
205 Offset 'value' does not exist on array{sourceType: string, expression: string, value?: mixed, sourceTypeSettings?: array{instance?: array<string, mixed>, storage?: array<string,
mixed>}}.
211 Offset 'value' does not exist on array{sourceType: string, expression: string, value?: mixed, sourceTypeSettings?: array{instance?: array<string, mixed>, storage?: array<string,
mixed>}}.
212 Offset 'value' does not exist on array{sourceType: string, expression: string, value?: mixed, sourceTypeSettings?: array{instance?: array<string, mixed>, storage?: array<string,
mixed>}}.
216 Offset 'value' does not exist on array{sourceType: string, expression: string, value?: mixed, sourceTypeSettings?: array{instance?: array<string, mixed>, storage?: array<string,
mixed>}}.
218 Offset 'value' does not exist on array{sourceType: string, expression: string, value?: mixed, sourceTypeSettings?: array{instance?: array<string, mixed>, storage?: array<string,
mixed>}}.
221 Offset 'value' does not exist on array{sourceType: string, expression: string, value?: mixed, sourceTypeSettings?: array{instance?: array<string, mixed>, storage?: array<string,
mixed>}}.
or they are, and we need to fix
Line src/PropSource/DynamicPropSource.php
------ -----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
37 Method Drupal\experience_builder\PropSource\DynamicPropSource::toArray() should return array{sourceType: string, adapterInputs: array<string, mixed>}|array{sourceType: string,
expression?: string, value: mixed, sourceTypeSettings?: array{instance?: array<string, mixed>, storage?: array<string, mixed>}} but returns array{sourceType: string, expression:
string}.
💡 Array does not have offset 'adapterInputs'.
💡 Array does not have offset 'value'.