🇺🇸United States @devkinetic

I second having to import the config twice, it's always drush cr; drush cim -y; drush cim -y.

This looks good to me, a simple but effective change.

I also ran into this. Adding the patch from 🐛 LanguageManager does not check against altered language_switch_links Needs work seems to help. I've not added the MR from this issue into my codebase. The issue only presents itself when logged out.

Furthermore, I've added a custom module the provides further link altering after this module, but found that I had to do two things to make it work:

1. Removed the typehinting within the function params. array &$links becomes &$links
2. Wrap my processor in a if (is_array($links))

So it seems that $links gets set to NULL instead of an empty array at some point by this module. As I said, I have not added this MR yest as it is not ready, but something to keep in mind when completing it, users may want to provide their own filtering afterwards!

function MYMODULE_language_switch_links_alter(&$links, $type, Url $url) {
  if (is_array($links)) {
    // custom code
  }
}

I ran into this bug as well, confirmed that the patch worked for me. I am also using "language switcher extended" module.

Thanks for the report. I've reviewed the patch and all looks well. marking as RTBC, and slated for merge.

I've put work into this topic over on 📌 Implement the current environment as a service and utilize plugins Active where current environment becomes a service and added a plugin manager to define the method of detecting the current environment. This way you can use whatever method you like, configure it, or even develop your own for more advanced scenarios. Unfortunately, it has not gotten any traction or feedback.

The pipelines are working correctly now, and I worked through the majority of the issues. I've also updated the issue summary with the remaining reported issues. What remains to fix could affect functionality of the module itself and warrants some real-world testing.

I've added the default gitlab ci file to the 2.x branch. Confirmed it is working now.

@james.williams I think I may have stepped on at least one of your changes from last night. I didn't realize you made a push before I applied my changes from local last night. Please review at your convenience.

Based on what is left, my major concerns revolve around the access logic and states where a method doesn't return anything. These could cause actual PHP errors and break the client side.

devkinetic → created an issue.

Thanks, this will improve the merge request process!

I've taken a first pass at the reported errors, and updated the issue description with what remains. I only updated stuff that should be non-breaking changes. What remains is mostly items that need to actually be tested in a working environment.

devkinetic → created an issue.

devkinetic → created an issue.

@james.williams Thanks for picking up the torch for this, glad my initial port is getting some love! This looks great so far. Couple of bigger picture things:

• We should cut a new branch using the new semvar format, deprecating the 8.x- prefix. Work there from now on.
• The new work needs to validated through coder/phpstan/phpcs.
• The next release should comprise of the above, and the 8.x branch should be set as unsupported.

I ran into this as well on a recent upgrade from Drupal 8 -> 9 -> 10. We ended up having to do a full database audit and found multiple issues, Before doing so we attempted the entire upgrade again so there is absolutely something missing in the upgrade path:

• batch table
  • missing primary keys
  • missing indexes
  • missing auto increment on bid
• block_content table
  • missing primary keys
  • missing indexes
  • missing auto increment on id
• block_content_* tables
  • missing primary keys
  • missing indexes
• some cache_* tables
  • missing primary keys
  • missing indexes
• users table
  • missing auto increment on uid

We solved the issue with the audit, and fixing the schema manually. I saw the referenced error about the users table, and can confirm that that update was ran but didn't fix things. I guess something to look out for when upgrading!

I just tested my previous patch against 4.0.17, which no longer works. Upon inspection, it seems to have been resolved by 🐛 Fix GIN Admin Toolbar Style: Secondary toolbar is overlapped by the 3px of the indicators border-top Fixed which I think this item is now a duplicate of and can be closed.

I have as well. This desperately needs a D10 release. It is the only item in my project without one, and has had 2 years to release these very basic fixes.

"repositories": [
        {
            "type": "vcs",
            "url": "https://git.drupalcode.org/issue/views_cm_current_state-3290445.git"
        },
        ...

composer require drupal/views_cm_current_state:dev-3290445-automated-drupal-10

I'd prefer to not bundle these changes together. The logger change should be a separate issue, and it should also use dependency injection.

Change record: Access checking must be explicitly specified on content entity queries →

Welcome and congratulations to @isholgueras! Thanks for opening this up in the first place to keep things rolling @e0ipso. Looking forward to getting this queue cleaned up.

devkinetic → created an issue.

Yes you are correct there is no field widget defined for IEF. You can try to create the file in the src folder and extend the existing widget. It's probably a lot like the others.

With that said I will create a branch for the work. But one thing I wanted to do was configure tugboat so it's easier to develop for all. I will try and do that tomorrow so we can get a test environment setup.

For anyone needing to upgrade to D10 now, I have created a fork on github drupal/field_label_plurals. It's just the latest 1.x with this patch applied.

"repositories": [
  {
    "type": "vcs",
    "url": "https://github.com/devkinetic/field_label_plurals"
  },
]

composer require drupal/field_label_plurals:dev-8.x-1.x

That is totally fine. It just had not been mentioned yet, and I assumed there were a few things needed to accomplish the migration that might warrant a ticket.

If I find some time I might take a crack at the simple stuff like documentation, updating the relevant files from the library, etc. I just started yesterday so I am not super familiar with this module or Splide itself.

With that said, rather than commit this to 1.0.x, it might be good to cut a new dev branch, and update this ticket so MRs will go against that instead.

devkinetic → created an issue.

devkinetic → created an issue.

I think a core feature was overlooked in the conversion. The initial permission, "access environment indicator" should solely drive the toolbar in the basic version, or the tab when using the toolbar. Having access to environments should only be taken into account building the selector or determining more advanced per-environment settings.

I haven't been able to work around this issue yet, but I'm still on D9. I ended up installing https://www.drupal.org/project/rebuild_cache_access → and instructing my authors of the one and only time to use it, and to schedule that during non peak hours.

I will be on D10 within the next month or so, and can certainly test this again at that point. I'd love to be able to remove that button.

I just did some experimentation with this concept. Here is a quick rundown:

\Drupal\media_contextual_crop_embed\MediaContextualCropEmbedService::addCropWidget
\Drupal\media_contextual_crop_embed\MediaContextualCropEmbedService::addCropWidgetForCk5

// get the image component for the media item
$source_field_config = $media_library_form_display->getComponent($source_field_name);
// modify getComponentConfig to pass it, instead of using preview_image_style from the plugin config
$component_data = $plugin->getComponentConfig($media_embed_element, $crops, $source_field_config);

\Drupal\media_contextual_crop_fp_adapter\Plugin\MediaContextualCrop\FocalPoint::getComponentConfig

if (isset($source_field_config['#type']) && $source_field_config['#type'] === 'image_focal_point') {
  $preview_image_style = $source_field_config['settings']['preview_image_style'];
  $preview_link = $source_field_config['settings']['preview_link'];
}
return [
  'type' => 'image_focal_point',
  'settings' => [
    'progess_indicator' => 'throbber',
    'preview_image_style' => $preview_image_style ?? 'crop_thumbnail',
    'preview_link' => $preview_link ?? TRUE,
    'offsets' => $default_values['data-crop-settings'] ?? '30,30',
  ],
];

Obviously you would have to adjust MediaContextualCropPluginBase.php and revert the last round of commits that introduced the "preview image style" on the ck plugin config.

The only question now is, right now it is grabbing the default form display, but in reality, we would really want the "media_library" display. But it would be nice to be able to choose. Maybe that is what the ck plugin config should allow you to select if anything. On the service side, we'd just have to update to:

$media_library_form_display = $this->entityDisplayRepository->getFormDisplay('media', $media->bundle(), 'media_library');

Or if configurable, it would be:

$media_library_form_display = $this->entityDisplayRepository->getFormDisplay('media', $media->bundle(), $data_source['display_mode']);

That seems to have done the trick, along with adding an empty image style to satisfy the plugin.

I do have to say though, we have focal point form display configuration on the media item itself, would it not make the most sense to retrieve that and apply it within the popup? This might be an issue for the focal point adapter itself. Something to think about.

devkinetic → created an issue.

I went with media_contextual_crop as it also allows for focal point within WYSIWYG as well.

devkinetic → created an issue.

I see that the "thumbnail" image style is currently hardcoded in \Drupal\media_contextual_crop_fp_adapter\Plugin\MediaContextualCrop\FocalPoint::getComponentConfig

Overriding that to "media_library" seems to do the trick in my case. I am able to apply a crop. At this point, I then tested swapping back to the "default" view mode within WYSIWYG, but an error is thrown as the default does not use an image style, but it is expected as the element now has "crop settings" from me just previously to that setting them on the other image style.

I also noticed that since the latest update my "crop" settings within text formats is empty. Not sure if this was intentional, as one would assume this would be where I would set that to be my "media_library" image style.

On a media ref field, when I edit the crop of an image, it uses the "media_library" image style, but when I edit the crop of an image within WYSIWYG it uses "thumbnail" image style.

I inspected my focal point widget settings and I can confirm I have everything set up to use the "media_library" image style. So perhaps the issue is now that focal point is falling back to default. It should instead be using the widget display settings.

On my site the "thumbnail" image style does use focal point, as it is used elsewhere on other entities.

I'm testing this today. This somewhat works, but needs a few tweaks.

1. When there are no plugins, LoadPlugins returns NULL. In implementation, it's always assumed that it will return an array of plugins or an empty array when none are found. This causes array countable errors. There are three places this method is used, we should review them. The simplist thing to do would be just make the method return an empty array instead of NULL.
2. When using the crop dropdown, we don't actually want that crop to show within the modal, just the end result. This would be after you select your focal point and press save to close the modal. The image shown in the modal should be the untouched source image.

To expand upon the second bullet further, think of it this way. I have an image that is 16:9 originally, and I want to use it as a cropped 4:3 using focal point in the WYSIWYG. After the update, if I select that 4:3 image style in the dropdown and press the crop button, the modal shows an already cropped image with the focal point. I can no longer see the entire image as the left and right have been trimmed away. Essentially, it should work like it does for a media field where you can see the whole image.

We have made progress because now the modal can know if should allow cropping or not, but it went one step farther actually using the crop in the modal itself.

I agree with you, disabling the button when you have an view mode selected that does not support cropping. Through my debugging I found drupalMediaElement['_attrs'].get('drupalElementStyleViewMode') which will contain the selected view mode in the CK5 plugin. If it is undefined, it's using the default from the filter configuration. We can model the ability to disable based on:

if(drupalMediaElement['_attrs'].get('drupalElementStyleViewMode') !== undefined) {

}

My setting looks like yours. My "Default" view mode does NOT use a crop. Could it be as simple as adding a check to make sure the string isn't empty? The code right now assumes that an image has a image style applied and that won't always be true.

I was able to work around passing the selected view mode to the dialog in CK5 JS. I'll be committing that code in a second.

devkinetic → created an issue.

devkinetic → created an issue.

devkinetic → created an issue.

@mlcn https://git.drupalcode.org/project/media_contextual_crop_field_formatter...

"drupal/media_library_media_modify": {
  "#3270150 Add alteration": "https://www.drupal.org/files/issues/2022-06-08/manage_crops_3270150-7.patch"
}

I actually just tested this last night and was able to get the contextual url in twig using {{ content.field_background_image|render|spaceless|striptags }} which allowed me to remove all the custom code I had in comment #2.

devkinetic → created an issue.

devkinetic → created an issue.

I've done some experimentation with converting the current environment code into a plugin system. I'm currently working through the ability to add settings for each implementation of the plugin, for example the patterns for the regex. This is apparently accomplished with EntityWithPluginCollectionInterface which is not documented very well.

As things stand the issue description stands to be updated.

DefaultPluginManager's construct has been updated:

parent::__construct(
  $subdir,
  $namespaces,
  $module_handler,
  $plugin_interface = NULL,
  $plugin_definition_annotation_name = 'Drupal\Component\Annotation\Plugin',
  $additional_annotation_namespaces = []
);

 The example should be updated to cover setting subdir correctly, and at least have some explanation around the plugin interface.

Upon further thought, I don't see why we couldn't extend the current environment deriver to be plugin based. We already have the ability to provide at least:

• The current settings.php config, with the addition of adding the machine name to enable permissions
• Simple host name matching
• Regex based host name matching

This would vastly simplify the upgrade path, as the settings.php plugin would be active by default.

After implementation, we would have to take another swing at permissions, to cover existing sites that do not have the machine name in settings.php.

I'd be willing to help out. This module has a lot of potential moving forward when it comes to permissions, features, and fixes.

I use it on many, many sites. I put together the shift ToolbarHandler, and am currently working on few other tickets here. I maintain a few other modules as well, hold security opt-in privileges, and know the module inside and out.

This looks good!

This looks good!

The url field the core field type, which uses core validation, nothing custom here. I found this issue #1444718: Warn users that underscores in (sub)domain names are not valid. → which details why Drupal does not support underscores in urls. With that said, we should follow suit in this module as well for consistency.

If you really need this, you can easily preprocess the links and add them in like so:

/**
 * Implements hook_toolbar_alter().
 */
function MYMODULE_toolbar_alter(&$items) {
    // do stuff with $items['environment_indicator']['tray']['environment_links']['#links']
}

See #3325119-7: Favicon always shows `(` after upgrading to 4.0.11 → .

This patch actually tracks with the original functionality. There is another issue 🐛 JS adds "(()" on mobile Closed: duplicate which swaps the title and the version around, but the truth is, all we need is the first character of the title, so no version is needed at all.

This looks good, it should be bumped up.

So this solution still requires editing settings.php to add the machine name of the config entity. I think it would be more useful if we did away with the settings.php stuff entirely and instead did regex/hostname matching on the active environment. I have an issue at 📌 Implement the current environment as a service and utilize plugins Active where I use that approach vs the MR on this issue. If you agree with me, I'd welcome closing this issue out and moving the work over there. One thing I really liked about this issue is you created a service, rather than stuffing ToolbarHandler with the logic.

devkinetic → created an issue.

Gotcha. The javascript (which is where the colors and favicon come from) is only included if you have access to the environment. That is a different issue than the issue description.

I'm pretty sure the issue here is the bug I found. It all centers around determining the currently active environment. There is no connection between the config overrides and the EnvironmentIndicator entities.

I'm working on a refactor that does away with the config overrides in settings.php, as the information is already in the config entities. Along with that is a refactoring of the permissions. There was a bug when the active environment could be blank. I took a long look at the permissions and they will be configured as follows:

- If a user "has access environment indicator" they can see everything.
- If a user does not have that permission, you mush opt the role into each environment via each respective permission.

When a user visits an environment in their browser, they must have permission to access the indicator for that particular environment, otherwise nothing will show.

@timwood I don't really understand your use case.

Thanks for all of the feedback, I will roll this in shortly.

I will be pushing a D10 release out shortly.

There is a more recent issue #3367093-8: Gin compatibility: Small screen vertical toolbar submenu items margin too big → where the css has been removed along with the js to support the latest versions of Gin, which is currently at 3.0.0-rc5.

Updated the patch in sync with the latest changes in 🐛 Gin Compatibility: Account for "open" vertical toolbar Closed: outdated . This patch and the MR on that issue differ in that this one removes the css , but both take into account the js update.

The correct solution would be to actually remove the negative margin altogether. It's no longer needed in more recent versions of Gin. See #3336512-6: Gin Compatibility: Account for "open" vertical toolbar → for details.

I have provided a patch that simply removes the rules altogether.

@tonka67 I reviewed the code and can confirm the updates have been added correctly. If I were you I'd throw in some debugging around https://git.drupalcode.org/project/views_jump_menu/-/blob/8.x-1.0-rc1/vi... and see what's going on. It could be your what you are inputting, perhaps it is already decoded?

D10 is right around the corner. Updating this module is trivial, any word on generating a D10 compatible release?

@natts Here is the change record → for reference.

The main issue is that the javascript didn't target the bubbled event data correctly, so some types of links would fail. That was fixed, and the question came to should there be a test to cover this situation?

When I looked at the existing tests, there was one test with two asserts for normal and anchor links. Adding in this new type of link would then make that single test have 3 asserts now, but also involve navigating backwards to the previous page after the second assert.

It was more straightforward to just split each link into it's own test.

This make sense, this module also needs a D10 version, so there will a 9 and 10 version released with the proper info metadata. I will also be dropping 8 from supported development. You can always use a previous version.

I agree that would be the easiest path to get things working.

TODO:
- Add settings page
- Add wrapper functions around drush calls
- Develop a mapping function for usage of settings
- Update readme
- consider adding dependencies to info from composer file

TBD:
- Backporting of functionality from newer versions of the module.

I have the latest version on my laptop, and will be pushing it up in the morning. We now need a settings page to mirror the options from the drush command and then to rework the check and report classes to work outside of drush.

As of now, it's a full fledged module with the addition of the info file metadata and module file so it shows up under manage extensions, and we have the actual report route showing a list of the reports. Once the classes are refactored we can uncomment the final code and it should work.

I did have an idea on how to easily detect drush execution vs the UI so we can get the existing code to work with minimal effort, and I'm going to be testing that out hopefully tomorrow if not next week.