Account created on 22 September 2015, over 8 years ago
#

Merge Requests

Recent comments

🇺🇸United States bvoynick

While I haven't set up a full test scenario - the site I encountered this on hasn't updated to 10.2 yet - I see the same as nterbogt, looks like the change in this MR was already introduced in the 10.2.x and 11.x branches.

🇺🇸United States bvoynick

The use case is downstream rendering or other things that may need cache metadata.

For example, let's say I render something using text from a config. Using core's ImmutableConfig object I could write code like this:

$config = \Drupal::config('my_config');
$render_array = [
  '#plain_text' => $config->get('text_property'),
];
CacheableMetadata::createFromObject($config)
  ->applyTo($render_array);

But what if I want to use translated_config? Since TranslatedImmutableConfig doesn't implement the CacheableDependencyInterface, currently my code would have to look something like this:

$translated_config = \Drupal::service('translated_config.helper')->getTranslatedConfig('my_config');
$render_array = [
  '#plain_text' => $translated_config->get('text_property'),
  '#cache' => [
    'tags' => [
      'config:my_config',
    ],
    'contexts' => [
      'languages:language_interface',
    ],
  ],
];

Which requires me to know the right metadata to hardcode.

Or alternately, I could make my own separate call to \Drupal::config('my_config') as in the first example, using the TranslatedImmutableConfig object for values only and not metadata.

But anyhow, that's the purpose of this feature request. To have the same cache-related "developer experience," when it comes to handling cache metadata, as with the core ImmutableConfig object, where one can simply send it to the right addCacheableDependency() or similar method.

🇺🇸United States bvoynick

I would also like to see this reopened. Config Split cannot address the issue that uninstalling Fastly & related Purge modules, in an environment lacks the relevant credentials secrets, produces a ton of noisy errors related to inability to purge.

🇺🇸United States bvoynick

Pushed the simple fix to a fork. Still needs a test

🇺🇸United States bvoynick

Updated MR to use the method, and to fall back to the old < 4.3.0 constant if the method does not exist.

🇺🇸United States bvoynick

A use case where I'm seeing this come up is importing a snapshot of a production environment elsewhere. I just had a content restoration process on a pre-production environment killed during its drush deploy, because runtime went over 15 minutes, due to the large amount of POSTs it was attempting to chew through. (The API key for this site is provided by environment variable, in Production only, and not stored in the Drupal database. Hence, even though I am using a production database with the processors enabled, there is nevertheless no API key in the context of restoring that database in this other environment.)

The site is set up so that `drush cim` / `drush deploy` will indeed uninstall all processors, and in fact all Purge & Fastly suite modules, when in a non-Production environment. But module installation state is in the database. When importing a Prod database elsewhere, the modules will have to be actively uninstalled during configuration import. Simply uninstalling these modules is triggering these POSTs, and resulting 404s, due to the lack of service ID and API key.

🇺🇸United States bvoynick

Here's an alternative workaround that adds media-specific logic to LinkitHelper::getEntityFromUserInput rather than revert the change to the saved URL.

Like the reversion patches, this too is probably not the approach the module should take long term, but it will help work around this problem on sites with the canonical route turned off.

🇺🇸United States bvoynick

This was indeed caused by 🐛 IEF does not invoke hook_entity_prepare_form Fixed . The problem is that the *_prepare_form hooks send both an $entity object directly, and $form_state. Any hooks that make use of $form_state->getFormObject() (and $form_state->getFormObject()->getFormEntity()) will get the form object and associated entity for the main/parent entity being edited, not the inline entity that needs to be prepared. This is why it's causing multiple issues across the board.

For instance, in the case of content_moderation incompatibility, Drupal\content_moderation\EntityTypeInfo->entityPrepareForm() interacts with the form object but in doing so uses the inline $entity passed to create a new revision and then call $form_object->setEntity(), overwriting the main entity associated with the form object with a new revision of the inline entity sent to the hook.

I spent some time trying to figure out a reasonable way to work around this, but I am not sure that one exists. It would be possible to temporarily override the entity on the form object with ->setEntity() before calling hooks for an inline entity, then set it back to the original afterward. But this still doesn't address that the form object itself will still be for the main form/entity. Logic that uses the form object, such as in content_moderation, will still be incorrect.

I think there's a strong case for going ahead with reverting 🐛 IEF does not invoke hook_entity_prepare_form Fixed for now. It simply is not working as intended. A path forward for invoking that hook properly must include instantiation/use of a form object specific to the inline entity and inline entity form.

🇺🇸United States bvoynick

The ticket description says removing a tag will work to stop bot activity going forward:

If you want to use this issue as a starting point to remove deprecated API uses but then don't want new automated patches, remove the "ProjectUpdateBotD10" tag from the issue and use it like any other issue (the status does not matter then).

Since this was the only thing the bot found, removing the tag and marking this fixed seems fine? But I'll leave that choice to you @jonathan1055

🇺🇸United States bvoynick

@steinmb This is one of the issues noted in the table in 🌱 Dealing with unexpected file deletion due to incorrect file usage Active . And I don't know of a reason work on this cannot proceed in parallel to the others there?

As the notes column there says regarding this issue: "No data loss, but irrepairable, therefore still critical."

🇺🇸United States bvoynick

Rerolled #34 as a fork for Drupal 10. (Note that one can get patches from Gitlab MRs or commits, e.g. https://git.drupalcode.org/issue/drupal-2879023/-/commit/c68aa376a7fc262886cb56d9ac762e4b06e6b174.patch)

Still needs tests before it can be MR/review ready.

🇺🇸United States bvoynick

bvoynick made their first commit to this issue’s fork.

🇺🇸United States bvoynick

The 2.0.1 release is indeed broken under Drupal 10.x due to reinstating the core/jquery.ui* library dependencies. See follow up issue 🐛 core/jquery.ui library does not exist in Drupal 10 Fixed

🇺🇸United States bvoynick

Opened MR 8 with an implementation.

New jquery_ui_library_info_alter() logic replaces core library jQuery UI files with dependency declaration(s) where possible. Where that is not possible, due to the site not having the sub-module for a particular JQUI library installed, the file definition in the core library is instead replaced using the file & definition from the jquery_ui library data.

At first I tried just replacing the files that are actually present in declared libraries with dependencies - and leaving any other JQUI assets in core libraries alone. However, I found this caused ordering problems. The jquery_ui module uses a -11 weight across the board for its JS files, and the recommended approach of dependencies within a network of library definitions to establish ordering. Core's drupal.autocomplete and drupal.dialog libraries - as well as all the jquery_ui* libraries included in 9.x - take a different approach, declaring everything in one library, and using lesser weights like -11.9 and -11.8 to order files within the one library. On a test site without certain submodules like jquery_ui_autocomplete enabled, that was resulting in dependent files that were still declared on the core library - like autocomplete-min.js - being included before core library dependencies like widget-min.js that are always going to be provided by the jquery_ui/core library. Which results in javascript errors.

So I switched to re-working all assets found in core libraries, by one method or another, based on a comprehensive mapping of jquery_ui.libraries.data.json.

🇺🇸United States bvoynick

It does still happen with aggregation disabled. I think it's down to there being two distinct assets in the two distinct libraries.

Removing core's asset definitions and substituting dependencies on the jquery_ui module's libraries sounds like a good path forward, thanks.

🇺🇸United States bvoynick

Thanks, issues with MR 4170 should be addressed and ready for re-review.

🇺🇸United States bvoynick

Updated issue title and summary

🇺🇸United States bvoynick

Thank you for the report & suggestion to override hasLinkTemplate, it's coming in handy for me today.

🇺🇸United States bvoynick

Also running in to some of these pathauto issues @danflanagan8 is reporting.

We're using this type of logic for similar reasons: it's a very centralized way to "turn off" the canonical-having nature of specific entities, of types that need to optionally have it. Using a toUrl() override plus a #pre_render callback on the link element, most things that construct links will automatically not link to these non-existent detail pages.

It's true that an access check service, or some other approach such as overriding the access() entity method, is also necessary in case someone does make their way to the canonical route anyway. Doesn't make it unworkable though, and 2-3 places are far easier to track down than every last place an item might be output: things like the Label entity reference formatter, which otherwise needs to be overridden, and all other such programmatic toUrl() calls. (For example, even in the default Content overview, nodes that implement these kinds of overrides show up without the title being linked. Can't remember if that's just from the toUrl() override or if that relies on the #pre_render, but otherwise It's pretty fantastic.)

🇺🇸United States bvoynick

Going to bump Priority to Major, since this is still as severe as 🐛 Overlay blocking image upload modal in Drupal 10.1 Fixed , it just probably affects fewer people.

🇺🇸United States bvoynick

Opened MR 18 implementing proposed short term fix.

🇺🇸United States bvoynick

Opened MR 6 with proposed resolution.

🇺🇸United States bvoynick

Opened MR 33 adding proposed clone statement.

🇺🇸United States bvoynick

Thank you for the patches catch!

At my workplace, we typically dev & deploy sites similar to what sime posted in #43, with aggregated assets cacheable by exact URL for a long time. I would be very happy to see #64 go in to core - and perhaps that is the best option, since it should also fix any regressions that any folks who don't define a version number are experiencing with 10.1.

Personally I also like the idea of a setting to opt in to including the deployment_identifier in hashes, since we usually do have one set. But I imagine not everyone does.

🇺🇸United States bvoynick

Looks like you're using an outdated version of drupal/coder. Make sure you use the latest release, and the custom PHPCS configuration included with the module.

🇺🇸United States bvoynick

MR #1 simply pastes copy from the project page. I'm open to adding a hook_help() if it would be valuable, but I'm curious to see what kind of in-UI documentation y'all would like and an example of it. (And an explanation of why it is needed beyond existing project page, README, and in-settings-page text.)

MR #3 does not even seem to be talking about this module. nader.mouldi, please do not take up other's time with inaccurate work.

🇺🇸United States bvoynick

I've run in to this as well. catapipper called it, a separate copy of core.css is loaded from the contrib module. In 10.1 it seems it's added at the bottom.

This is also discussed here Issue #1461322 🐛 Fix AJAX add_css – insert the needed CSS assets after the already-inserted ones Needs work in #32, work on a change to AJAX behavior produced this before.

I am guessing the fix would likely lie in the jquery_ui module, since this technically does not seem to be a problem with core.

🇺🇸United States bvoynick

Opened an MR that addresses this issue by moving the library attachment to this module's block twig template.

🇺🇸United States bvoynick

I have also noticed this following an upgrade to 10.1.0. Don't recall seeing it prior to 10.1.x.

🇺🇸United States bvoynick

I didn't read docs closely enough, reusing an MR for a different branch is not possible.

Opened new MR #4170 for the -11.x branch.

🇺🇸United States bvoynick

I've rebased claudiu.cristea's MR (in a separate -11.x branch) and added a test.

If the MR can be changed to point to that branch, I think this would be ready for review.

🇺🇸United States bvoynick

bvoynick made their first commit to this issue’s fork.

🇺🇸United States bvoynick

Why is this marked as fixed? In the 6.2.x branch, the file's last commit was still 4 months ago. Longer in other branches.

🇺🇸United States bvoynick

Also ran in to this. Gin defines two separate libraries for dialogs, so you'll need three overrides:

libraries-override:
  claro/claro.drupal.dialog: false
  gin/dialog: false
  gin/gin_dialog: false

With the addition of gin/dialog to the list, I'm no longer seeing any Gin dialog-related CSS in the public theme using these overrides.

🇺🇸United States bvoynick

Besides core_version_requirement, the only other issue reported by Upgrade Status 4.0.0 is this:

Class Drupal\field_config_cardinality\Plugin\Field\FieldWidget\CardinalityMediaLibraryWidget extends @internal class Drupal\media_library\Plugin\Field\FieldWidget\MediaLibraryWidget.

This isn't an incompatibility with Drupal 10. The extended plugin has not changed between 9.5 and 10.0, and I just tested this widget with field_config_cardinality with core 10.0.5.

I have not tested every other field type with special handling - it does look like there are conditions for a few like managed file - but preliminarily it looks to me like it may be fine to commit the patch from #2 as is.

🇺🇸United States bvoynick

bvoynick created an issue.

🇺🇸United States bvoynick

Fix/feature removal committed to a new 2.0.x branch, since this is a breaking change.

Production build 0.67.2 2024