🇺🇸United States @angrytoast

+1 for @Berdir's approach in #19. Since the entity field definitions are already statically and persistently cached, it's a much simpler way of getting at the necessary field names and removing the repeated + slow field_storage_config loadByProperties call.

With this, the static cache in WebformEntityReferenceManager::getFieldNames doesn't matter as much. Basic local benchmark (our own site, not a standard Drupal install) on a larger page consisting of 68 paragraph references, 5 unique bundles, shows on average ~0.0008s faster with the static cache. I think it's not a meaningful difference and can be removed to reduce code.

Thoughts?

https://www.drupal.org/project/drupal/issues/2519362 📌 Redesign the menu link add form to be less overwhelming Needs review recently went into 11.x; it includes a form-two-columns abstraction which looks similar to what this issue is trying to achieve. Since that's actually in core and now a precedent, does it make sense for the work here to change and adapt to it?

I can reproduce the issue with a fresh site install on 10.2 on the standard profile.

Adding a link field to the default Article content type and then configuring it on the content type's default display view mode shows the 80 character trim regardless of if it is the "Link" or "Separate link text and URL" option. It looks like this is defined as such in the respective plugins:

• <a href="https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/link/src/Plugin/Field/FieldFormatter/LinkFormatter.php#L79-87">LinkFormatter::defaultSettings</a>
• <a href="https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/link/src/Plugin/Field/FieldFormatter/LinkSeparateFormatter.php#L28-34">LinkSeparateFormatter::defaultSettings</a>

Digging into git history, it looks like this was added way back in 2013 at the early stages of Drupal 8 development. The issue doesn't have any clear reasons on why 80 characters is the default: https://www.drupal.org/project/drupal/issues/1796316 →

Back to the main topic: I agree that the default trim length causes more confusion than the good it does. This is especially relevant in REST views when you try to output a link, if your sample data doesn't result in a trim, it is hard to tell this will become a problem. I think removing it from the plugin defaults is a good idea.

Adding for consideration, regarding the risk factor mentioned in #9: Using !important on .hidden or [hidden] affects not only CSS overriding but also javascript behaviors, using jQuery or vanilla js won't work as expected, e.g. if something is set to:

[hidden] {
  display: none !important;
}

and given a DOM structure like:

<div id="my-element" hidden="true"> ... </div>

Then jQuery('#my-element').show(); or document.getElementById('my-element').style.display = 'block'; will not work. The only way it'll work is to toggle the class or the attribute and this will be a change in expectations of how something should work.

Setting issue back to 11.x as the active development branch and back to Feature request as this is net new functionality and not a bug.

Opened a new MR -- https://git.drupalcode.org/project/drupal/-/merge_requests/5975 -- against 11.x as it wasn't clear how to change the target of the existing MR against 9.3.x

Changes include:
- Starting from the patch state of #36
- Updates to more SharedTempStore::set related methods that are also affected by $expire
- Updated tests for Private/Shared Tempstore classes
- Typehinting for params and returns in affected code

angrytoast → changed the visibility of the branch 11.x to hidden.

angrytoast → changed the visibility of the branch 2883521-expose-expire to active.

angrytoast → changed the visibility of the branch 2883521-expose-expire to hidden.

angrytoast → changed the visibility of the branch 2883521- to hidden.

Updated with a MR and incorporates feedback from #29 regarding no need for the case-insensitive header logic update.

Also updated with typehints for params + returns on affected / new methods as requested by #46.

angrytoast → made their first commit to this issue’s fork.

Addressed feedback from #133. Not sure about MR resolving policy, setting this back to Needs Review to get eyes on it.

Updated to use a MR because the patch triggered DrupalCI process looks to be failing consistently due to disk space issues in the Jenkins based builds.

Same changes as patch in #126, interdiff is represented in this commit

Adding an updated patch + interdiff with changes based on most of the feedback in #125. Specific points:

1. The D7 MigrateFieldFormatterSettingsTest does not have any cases of file_url_plain which differs from the D6 version. Not sure on how to proceed here as it seems like it'd be a bigger change in the test migration?

8 + 9. It seems like the public vs private paths ultimately ended up with the same assertions so that the only difference came down to private triggering a logout in the test. That didn't seem necessary as the tests were asserting on the existence of the url.site cache context which should exist regardless of auth state. Ended up removing the if paths.

Also updated the Change Record with more descriptions and Issue Summary to include screenshots.

Just came across https://www.drupal.org/project/drupal/issues/2811043 ✨ File formatter render absolute url to file Needs work (File formatter render absolute url to file) which looks like a more complete solution that includes both file + image absolute link options, tests, and cache context considerations.

We should focus on getting that issue merged instead as it is more complete?

Regarding comments #28-30 that suggest using type hinting, wouldn't this cause issues for code out in the wild that call the method and currently does not cast the first $timestamp parameter? If we go down this route, should it have a BC layer to warn and eventually enforce the parameter strict typing?

IMO the current approach in https://git.drupalcode.org/project/drupal/-/merge_requests/3254 is more accommodating and would cause less issues for existing implementations.

Thoughts?

Gotcha, updated the MR with a basic kernel test to assert the deprecation messaging and drafted up a change record.

That looks like it should be enough? Putting back in needs review.

Updating to a MR against 11.x and incorporating the BC trigger_error warnings per #21 and https://www.drupal.org/about/core/policies/core-change-policies/drupal-d... → .

Clarification on tests; as of 2023-12-15, https://www.drupal.org/about/core/policies/core-change-policies/drupal-d... → states

Tests are not required where the test would only be asserting that a deprecation message is triggered

The suggested tests would fall under only asserting a deprecation message? Is it required?

angrytoast → made their first commit to this issue’s fork.

Updating with a related issue link to a change that provides an "Use absolute link" option at the image URL field formatter level. This may be a viable solution?
https://www.drupal.org/project/drupal/issues/3041402 ✨ Add option absolute url in formatter URL to image Needs review

This is exactly the change we needed for exposing image field data to external API consumers. Thanks to all who have contributed!

I took a first stab at a change record using a REST json output as the example. Hopefully that is sufficient and helps this one get merged.

https://www.drupal.org/node/3408945 →

angrytoast → created an issue.

Updated MR to be against 11.x

As best as I can tell by looking at git blame, this happened in https://git.drupalcode.org/project/drupal/-/commit/40d5f16f5577d072228b9... for https://www.drupal.org/project/views/issues/731662 → , specifically between patch 129 and patch 136.

• 129 didn't have the duplication yet:
• https://www.drupal.org/project/views/issues/731662#comment-5682370 →
• https://www.drupal.org/files/731662-hybrid-filters.129.patch →

• By 136, about 2 months later, the duplication was introduced in the re-rolled patch in function build_group_form
• https://www.drupal.org/project/views/issues/731662#comment-6033342 →
• https://www.drupal.org/files/731662.views_.136.patch →

I didn't see any comments between the 2 points that suggests this should have be 2 separate code paths; given that it was a re-roll and the size of the patch, it seems like an unintentional code duplication.

angrytoast → created an issue.

The premise here seems odd. ExposedFormCache is treating the exposed form data as if it was a request level static cache. In the current state, this would only be beneficial if the view display was rendered more than once on a page?

Should this cache data be in a persistent cache bin instead?

Note: this looks like a dupe of https://www.drupal.org/project/entity_browser/issues/3350169 🐛 Fix PHP 8.2 deprecation issue with WidgetSelectorBase-class Fixed

Continuing work from #20, interdiff is still against #17.

Triggering test run

A number of changes from the last patch in #17:

• Renumber the update hook from 8105 to 8202 since this is targeting the 8.x-2.x branch and there is already a 8201
• Ensure that the entity_print config is installed where necessary and updated with necessary entity type ids for tests
• Remove an odd injected $this->stringTranslation in EntityPrintPermissions as is it never used directly and the string translation functionality is already covered by use StringTranslationTrait
• Fix up a couple of code standard recommendations

This doesn't include any net new tests. Just trying to get it to a passing state against the existing tests.

Adding a patch for the dynamic class properties issue

Updating the issue summary and tags to more accurately reflect the PHP 8.2 compatibility issue.

Here's a quick patch for the issue.

angrytoast → created an issue.

+1 RTBC. This corrected issues on our site with adding net new content that used the HMS field type and editing content without a value.

This should be in a latest numbered release.

angrytoast → created an issue.

After experimenting a bit, it looks like we can further simplify code with a public MenuLinkContent::getEntity(). The entity retrieved via the method is already the correct translation, so we can skip the entity.repository call.

Adding an updated patch with the simplified logic.

Whoops, missed a spot in the patch. Here's the updated one.

Agree with the assessment in #5 and #8. Here's a starter patch against 3.0.x that addresses those points:

• Deprecation message is only shown on the formatter summary
• Warnings are logged, not shown as a message on viewing the formatter
• Add hook_requirements warning on the status page if deprecated instances are detected
• Add update hook to update the affected configs. I just picked 8001 as the number since the latest info.yml claims 8.x compatibility and there has never been an install file for this module

This patch addresses core.entity_view_display and views.view config as the obvious spots where there may be affected field formatters. Not sure of other potential cases.

Following up again, here's a slightly different approach that limits the drupalSettings addition by not using hook_page_attachments. Instead it adds them in the respective plugin files so that it's only added when needed, which feels better.

That said, we only use the filter functionality. I can confirm it works there, but can't confirm for the AceFormatter use case. I'm also not familiar with editor plugins so the base_path isn't added there to drupalSettings and will need more work.

A quick update to the previous patch in #3. Currently ace_editor_lib_path doesn't cache its discovered library path so it must run through the filesystem each time it is run. Because the patch adds a hook_page_attachments implementation to populate drupalSettings, it's helpful to cache the path for future use.

A better solution would be to introduce a service that can be used in both procedural code and the various plugins this module provides but that seems like a bigger refactor that should be tackled separately.

Adding a patch for this bug that looks to work for both 1.4.0 and 2.0.0-alpha1 as of 2023-07-05.

It adds a general setup.js that calls ace.config.set('basePath') to make sure it has the value of the discovered library path from the module.

We tested out the latest changes in MR4 and see the same issues with Column not found: 1054 Unknown column 'mlcd.link_override__options' in menu_link_content_field_revision.

This is with:

• Drupal 9.5.8
• PHP 8.1.14
• MariaDB 10.4.27
• Started using translatable_menu_link_uri at 1.2.0, updated to 2.0.0, and now trying to update to latest 2.x-dev commit dev-2.x#822d9712

I looked through the issue queue a bit and found https://www.drupal.org/project/translatable_menu_link_uri/issues/3221161... 🐛 Mismatched entity and/or field definitions RTBC which contained an update hook example that worked. It looks like it should be used in place of some of the changes here. I've opened https://git.drupalcode.org/project/translatable_menu_link_uri/-/merge_re... which I could run with no issues.

I've also tested resetting the schema version back to 8001 to re-run both and it doesn't appear to have any issues. However it'd be good for others who have already ran 8001 to see if there are issues.

angrytoast → made their first commit to this issue’s fork.

The core issue ✨ Set MenuLinkContent getEntity to public visibility Fixed to expose \Drupal\menu_link_content\Plugin\Menu\MenuLinkContent::getEntity() looks like it will finally happen which makes Option 1 the best approach.

This is to update the patch to work with the latest 2.x branch code. It also adds a more specific check for MenuLinkContentInterface instead of just EntityInterface because we are looking for a menu link content entity.

Question: Per #18 ✨ Set MenuLinkContent getEntity to public visibility Fixed

Also if the method is going to be public it should probably be added to MenuLinkContentInterface.

Is this accurate? We're looking at the menu link plugin class, which extends MenuLinkBase which implements MenuLinkInterface

Should it be on MenuLinkInterface or MenuLinkContentInterface?

OK. Builds are green, that's a good start. Marking as needs review again to get feedback on updated tests.

Small update to the patch, missing a trailing comma which caused CI build fail.

Revisiting this one. After looking through history and specifically at comments #64 🐛 ManyToOneHelper ignores group configuration for some cases Fixed and #96 🐛 ManyToOneHelper ignores group configuration for some cases Fixed , adding patch to hit the additional test cases.

I'm not certain about modifying and saving the view repeatedly in the test. Thoughts?

I see that the newly added tests are in Functional tests Drupal\Tests\system\Functional\Datetime\DrupalDateTimeTest, however there are already existing unit tests in Drupal\Tests\Component\Datetime\DateTimePlusTest. This looks more appropriate to add to the unit test class as they would be better grouped and much faster?

Per #32, update issue summary with default template and relevant details

Add the patch changes in #16 to the existing MR with a minor change. Modified the field_storage_config<code> entity query lookup in <code>:: getParagraphFieldNames to target paragraph fields and get fields for all entity types at once instead of doing it gradually per type.

https://git.drupalcode.org/project/webform/-/merge_requests/122/diffs?co...

Took a stab at both the release note and change record → . Also rebased the MR.

Putting this back in Needs Review for further comments.

Adding Release note snippet per review request.

It looks like currently when loading config on a multilingual site, LanguageConfigFactoryOverride::loadOverrides will always run even if the current negotiated language is the default language. This will check for a language override that won't exist because the default language doesn't have a collection--if my site's default language is en, the imported config values will be under empty collection '', not language.en.

As-is, it tries to load a config by language.en, doesn't find any overrides, and then writes this to cache as a serialized FALSE value, which is then filtered and discarded in the process.

It seems safe to skip the whole process if we are looking at the default site language?

angrytoast → made their first commit to this issue’s fork.

Thanks for the patch. We have a site that uses paragraphs heavily and are seeing performance issues in this area.

I looked over the changes and it seems like there are a couple of improvements that can be made, how do these sound?

1. In ::getFieldNames, instead of caching by the {{entity_type}}-{{entity_id}}, cache by {{entity_type}}-{{bundle}}. Especially for paragraphs, this accounts for multiple instances of a paragraph bundle on a page which is more likely given the reuse nature. As-is, the class cache var $fieldNames ends up looking like:

node-1111 => fields
paragraph-1001 => fields
paragraph-2002 => fields
paragraph-3003 => fields
... etc

Instead it can be:

node-bundle => fields
paragraph-bundle1 => fields
paragraph-bundle2 => fields
... etc

The latter will likely have a better hit ratio for paragraphs.

2. Cache field names returned in ::getParagraphFieldNames by entity type.
This one gets called often with lots of paragraphs. We're actually seeing a lot of time spent here in the field_storage_config look up for paragraph field names. Since the field names are fixed per entity type and unlikely to change, thinking this is useful to cache both at the request level but also in Drupal cache for future requests since these aren't likely to change, and skipping the field_storage_config load helps across multiple page requests.

angrytoast → created an issue.

The MR code in https://git.drupalcode.org/project/openid_connect/-/merge_requests/41 works well. For context, this works well for our use case which initiates login from the idP (Okta, in this case) and it fixes a confusing experience where the user may have a Drupal session, but then come through the idP and get an error screen.

Attach the correct interdiff between patch #105 and #121 as txt instead of a patch file.

Reading through earlier comments again, I agree with comment #95 🐛 ManyToOneHelper ignores group configuration for some cases Fixed in that the views_test_filter introduced in earlier patches isn't necessary--the newly added test uses an already existing view config test_filter_taxonomy_index_tid.

Updating the patch to remove the portion that adds the core/modules/views/tests/modules/views_test_filter module.

That's looking better, adding the interdiff for 105 and 118

Of course patch 116 "worked on my local machine" but fails on Drupal CI...

Trying out a different approach in the test to avoid counting elements on the page and instead checking for presence of the expected node titles in the test view output.

Updating the patch per xjm's comments from #115.

It looks like the reason for the fails is due to the change of default test themes from Classy to Stark ( https://www.drupal.org/project/drupal/issues/3248295 → ) which no longer inserts the view-content class the test originally relied on.

This changes the selector to CSS based on the views-element-container class which is added directly in Drupal\views\Element\View::preRenderViewElement rather than in a theme template.