alexpott → created an issue.
@marcosano has pointed out that entity embed is a problem. Entity embed possibly will add links to other entities and we wouldn't want these usages registered as part of the entity with text field... so yeah #4 is wrong-headed.
That said I think it points to the fact that we could reorganise the plugin relationships so we only have to create the dom for each text field once.
I've been thinking about this issue and it makes me wonder if we should be maintaining:
- \Drupal\entity_usage\Plugin\EntityUsage\Track\EntityEmbed
- \Drupal\entity_usage\Plugin\EntityUsage\Track\HtmlLink
- \Drupal\entity_usage\Plugin\EntityUsage\Track\MediaEmbed
- \Drupal\entity_usage\Plugin\EntityUsage\Track\LinkIt
- \Drupal\entity_usage\Plugin\EntityUsage\Track\CkeditorImage
I think we should have a single text field plugin that uses the processed text (and summary if available) to determine usage. The plugin should support entity usage discovery from both the URL and the data-entity-uuid and data-entity-type attributes.
This approach would have the following advantages:
- No double reporting from LinkIt/ EntityEmbed / CkEditorImage / MediaEmbed - these plugins are all essentially the same - they use the attributes to determine if there is a link
- A single plugin working through all the links is more performant and less code to maintain
- Support for any esoteric filter that adds links in a way not covered by the existing plugins
Now to add test coverage...
@phenaproxima I added test coverage of an existing dependency. I've also got test coverage of moving a dep from require-dev to require and not moving a dep from require to require-dev :)
alexpott → created an issue.
alexpott → created an issue.
alexpott → created an issue.
alexpott → created an issue.
alexpott → created an issue.
If we changing the 10.3 database dumps because of invalid schema this points to us needing an update function. I think we should not be changing the dumps.
Also I think we should be adding a new constraint that works like callback but uses the class resolver service to my more flexible.
Committed 2778882 and pushed to 11.x. Thanks!
alexpott → created an issue.
alexpott → created an issue.
I've tested this solution with easy breadcrumb as described by 🐛 Adding url.path to breadcrumb cache context results in every 404 page getting a different dynamic page cache entry Active and it works fantastically. This is a fantastic change which will improve the cacheability of any block that is using the response status condition.
How does this work? Doesn't this need to be something we can determine from the request? Which we can't do for the response code isn't this going to end up in the page cache too?
Here's a decent blog post on the subject - https://drupalsun.com/brian/2020/12/20/how-drupals-dynamic-page-cache-de...
alexpott → created an issue.
@zengenuity yep that's the fix - nice!
So what's super interesting is the white color is coming from a user agent style sheet... but this is the same on all browsers I have available - brave, safari and firefox....
user agent stylesheet
table {
border-collapse: separate;
text-indent: initial;
line-height: normal;
font-weight: normal;
font-size: medium;
font-style: normal;
color: -internal-quirk-inherit;
text-align: start;
border-spacing: 2px;
white-space: normal;
font-variant: normal;
}
I wonder if this is mac thing... ah,.... if I look in the HEAD section of the iframe I see
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<meta name="viewport" content="width=device-width, initial-scale=1">
<meta http-equiv="X-UA-Compatible" content="IE=Edge">
<meta name="format-detection" content="telephone=no, date=no, address=no, email=no">
<meta name="x-apple-disable-message-reformatting">
<meta name="supported-color-schemes" content="light dark only">
<meta name="color-scheme" content="light dark">
<!--[if mso]>
<noscript>
<xml>
<o:OfficeDocumentSettings>
<o:AllowPNG/>
<o:PixelsPerInch>96</o:PixelsPerInch>
</o:OfficeDocumentSettings>
</xml>
</noscript>
<![endif]-->
<!--[if (gte mso 9)|(IE)]>
<style>
sup{font-size:100% !important;}
</style>
<![endif]-->
</head>
If I remove
<meta name="supported-color-schemes" content="light dark only">
<meta name="color-scheme" content="light dark">
Then I can see the text... I wonder what is adding them. Ahhh I found... so I have dark mode enabled on my computer... if I do then I can't see the text. If I switch off dark mode then I can see the text.
This is an easy email bug. Given it is adding a background color of #FFF to the table displaying the email it has to set the font color as it cannot use the OS defaults as these depend on whether the OS is in a dark mode or not.
alexpott → created an issue.
I think we should prevent the UUID key and ID key being changed. It'd be a really really odd thing for this to be able to do. We prevent config install and sync from changing UUIDs keys - see \Drupal\Core\Config\Entity\ConfigEntityStorage::updateFromStorageRecord()
alexpott → created an issue.
alexpott → created an issue.
alexpott → created an issue.
alexpott → created an issue.
Turns out that 📌 Save paragraphs as part of a host entity save Active introduced a bug where the latest revision was not used for editing content with paragraphs. This is wrong - we always have to change the latest revision because we cannot change both the published and the latest revision.
Hmmm this is not possible because the locale part happens before config import... from the docs
/**
* Ensures that translations are updated as part of a drush:deploy command.
*
* Translations are imported prior to configuration to ensure that the
* configuration is the final say as it is very confusing when configuration
* changes after running an import.
*
* @hook post-command updatedb
*/
public function postUpdateDbHook(int $result): void {
And I think we need to be in maintenance mode during config import.... therefore closing this as won't fix. I think the real fix is to make the locale code much faster...
alexpott → created an issue.
The test fail is unrelated to this issue and is fixed by 🐛 RouteTest::testResolveNode() fails on HEAD Active
alexpott → changed the visibility of the branch 3515177-routetesttestresolvenode-fails-on to hidden.
This can easily be fix in the GraphQL module so moving over there.
Specifically the changes to tests/src/Traits/DataProducerExecutionTrait.php
This is caused by a recent commit to GraphQL - https://git.drupalcode.org/project/graphql/-/commit/ebb04a7676d3cf43a3b3...
alexpott → created an issue.
alexpott → created an issue.
Note that now we have the composer command linked up for a core checkout this code can easily be tested. On a core checkout on this MR branch you can do:
# Require a recipe. It will not be unpacked because we disable automatic unpacking in the core root composer.json
$ composer require drupal/events
# Unpack the recipe
$ composer drupal:unpack drupal/events
If you want to test automatic unpacking you can update the root composer.json add set on-install-and-update to true in the extras section.
alexpott → created an issue.
phenaproxima → credited alexpott → .
Still can't update to it via the UI because the updater won't update recipes :)
Problem 1 - Root composer.json requires drupal/drupal_cms_olivero 1.1.0 (exact version match: 1.1.0 or 1.1.0.0), found drupal/drupal_cms_olivero[1.1.0] but these were not loaded, likely because it conflicts with another require. Problem 2 - drupal/drupal_cms_starter is locked to version 1.0.2 and an update of this package was not requested. - drupal/drupal_cms_starter 1.0.2 requires drupal/drupal_cms_olivero ~1.0.1 -> found drupal/drupal_cms_olivero[1.0.1, 1.0.3] but it conflicts with your root composer.json require (1.1.0).
But at least composer update from the CLI now gets me to the latest theme release and should solve the problem for a while.
Here's a discussion of what the PSR stuff means... https://stackoverflow.com/questions/14962667/what-is-a-line-length-soft-...
I think that another part of this issue is that we only implementing a line length limit when it has an array. I think we need to revisit all of our line length limits and compare them with the PSR standards (which are more modern but still getting on now). The current PSR-12 guidelines on line length are:
There MUST NOT be a hard limit on line length.
The soft limit on line length MUST be 120 characters.
Lines SHOULD NOT be longer than 80 characters; lines longer than that SHOULD be split into multiple subsequent lines of no more than 80 characters each.
See https://www.php-fig.org/psr/psr-12/
I have no idea why we have a particular rule of arrays... it's a bit odd.
It'd be great if this could make it's way back to 1.0.x releases too.
But this is perfect.
phenaproxima → credited alexpott → .
No it is not auto-formatted but it passes out CS checks.
The point is that applying this rule to core has resulted in changes like
- ['entity.user.collection', [['entity.user.collection', 'user.admin_permissions', 'entity.user_role.collection', 'user.role.settings']]],
- ['user.admin_permissions', [['entity.user.collection', 'user.admin_permissions', 'entity.user_role.collection', 'user.role.settings']]],
- ['entity.user_role.collection', [['entity.user.collection', 'user.admin_permissions', 'entity.user_role.collection', 'user.role.settings']]],
to
+ [
+ 'entity.user.collection',
+ [
+ [
+ 'entity.user.collection',
+ 'user.admin_permissions',
+ 'entity.user_role.collection',
+ 'user.role.settings',
+ ],
+ ],
+ ],
+ [
+ 'user.admin_permissions',
+ [
+ [
+ 'entity.user.collection',
+ 'user.admin_permissions',
+ 'entity.user_role.collection',
+ 'user.role.settings',
+ ],
+ ],
+ ],
+ [
+ 'entity.user_role.collection',
+ [
+ [
+ 'entity.user.collection',
+ 'user.admin_permissions',
+ 'entity.user_role.collection',
+ 'user.role.settings',
+ ],
+ ],
+ ],
This is not an improvement. It was easier in the original formatting to see that we had three test cases expecting the same tasks on the three different routes. With the current formatting that is much much harder to discern.
I'm working through the comments on the MR.
I've added a new drupal:unpack RECIPE/PACKAGE command to help existing sites.
Still to do:
- Add test coverage of the new command
- Check existing test coverage to make sure it is good
- Write a change record
- Add plugin to core's root composer.json and the project templates.
I've rebased the MR on top of 11.x to make it mergeable.
This has just made me change:
$this->assertEquals(['entity_test.entity_test_mul_bundle.test', 'language.content_settings.entity_test_mul_with_bundle.test'], $permissions['translate test entity_test_mul_with_bundle']['dependencies']['config']);
to
$this->assertEquals([
'entity_test.entity_test_mul_bundle.test',
'language.content_settings.entity_test_mul_with_bundle.test',
], $permissions['translate test entity_test_mul_with_bundle']['dependencies']['config']);
It's not an improvement. I think we should have reviewed all the lines that the updated rule with 120 length was suggesting we change. I'm not convinced that these line length rules result in good code and are appropriate for standards. I opened ✨ Drupal.Arrays.Array.LongLineDeclaration make me write less readable code Fixed to try to get us to consider this particular standard and that resulted in changing a limit from 80 to 120 which while better does not get to the real problem which is that writing standards on line length is hard and Drupal's current standards are out-of-date and need revisiting so that they make sense with the current state of code rather than enforcing them on our code. Our standards were written my Drupal was largely procedural. See https://lkml.org/lkml/2020/5/29/1038 for a longer discussion of why arbitrary line length limits are bad.
Fixed the coding standard sadness.
alexpott → created an issue.
alexpott → created an issue.
catch → credited alexpott → .
This is a great fix. Lots of hardcoded overrides removed and works a treat. I'm using this MR on a site with exported commerce products & variations and it cleans up a load of stuff.
alexpott → created an issue.
Functional tests should use \Drupal KernelTests should not. so the issue title is incorrect. Sure we can inject services correctly into the controller...
> scaffold does not run if you install Drupal from core git repo
Well this issue resolves this for this case because we're calling the preAutoload dump from core code now so it's way better. I don't think scaffolding has to run but there being a common preAutoload dump is necessary and a good thing. This issue introduces that and will make 🐛 Wrong DRUPAL_ROOT with non-standard code structure Needs review easier as you'll be able to rely on the presence of the constant.
It would be great to have some reviews on this...
I've written some dumb code in the \Drupal\package_manager\PathExcluder\UnknownPathExcluder - but I think the plan to add a command to the scaffold plugin to produce a list of files is a good one. But this is a starter for 10, so to say.
And we also need to account for additional packages that can scaffold. The root composer and Drupal core are always permitted to scaffold.
Example: Permit scaffolding from the project `upstream/project`
```
"name": "my/project",
...
"extra": {
"drupal-scaffold": {
"allowed-packages": [
"upstream/project"
],
...
}
}
```
We need to make changes in \Drupal\package_manager\PathExcluder\UnknownPathExcluder::getScaffoldFiles() and it needs to account for scaffolding options like
"[web-root]/sites/default/default.settings.php": {
"mode": "replace",
"path": "assets/sites/default/default.settings.php",
"overwrite": true
},
"[web-root]/sites/default/settings.php": {
"mode": "replace",
"path": "assets/sites/default/settings.php",
"overwrite": false
},
"[web-root]/robots.txt": {
"mode": "append",
"prepend": "assets/robots-prequel.txt",
"append": "assets/robots-append.txt"
},
This is now a core issue as package manager is in there now.
This is a major bug because it break automatic updates and you need a way to append stuff to .htaccess if you have custom rules in there because otherwise an automatic update will override your .htaccess changes.
Okay there is a work around. You can do vendor/bin/drush cset package_manager.settings include_unknown_files_in_project_root 1
But this is still a major bug. We should support copying files that are used in scaffold without having to set anything additional. And as there is no settings form and the error message doesn't point you to this it would be extremely hard for someone to solve this if they can't just ping @catch.
This is a duplicate of ✨ When determining scaffold file mappings, determine them consistently and completely, accounting for overrides Active going to close and move that issue to core.
Okay there is a work around. You can do vendor/bin/drush cset package_manager.settings include_unknown_files_in_project_root 1
But this is still a major bug. We should support copying files that are used in scaffold without having to set anything additional. And as there is no settings form and the error message doesn't point you to this it would be extremely hard for someone to solve this if they can't just ping @catch.
alexpott → created an issue.