🇫🇮Finland @sthomen

Lets try again to fix my blunder, setting back to needs work.

I just received a spate of these attempts to some of our sites this weekend and I find it a little alarming that drupal assumes the type of the input based on the rendered element that the input happens to correspond to.

Looking at performRequiredValidation(), the maxlength check seems to be done a little too early, perhaps something like the attached patch instead? With it, I get a 400 instead of a crash and error 500 when sending an "array" instead of the expected in the query string to /user/password.

Given that this is a pretty critical piece fo safety I would not trust myself to get this right, more eyeballs please :-)

Thank you for cleaning things up, I had no idea I was being so messy. Time to get started with phpcs.

The large removals from SvgImageWidget are because the widget used to inherit directly from FileWidget and duplicated large swathes of ImageWidget. My change is to directly inherit from ImageWidget and just tweak the things that are needed (i.e. adjust the validators and change the theme from image_style to just image on the preview(since an SVG isn't a bitmap, it makes little sense to try to pass it through the image style path).

There's aready an existing issue (#3420037) which is about inheriting from ImageWidget, I didn't know about it at the time I did the change, hopefully this won't cause a merge issue.

The optgroup appears to be by design in ViewfieldWidgetSelect::ajaxGetDisplayOptions

Yikes! It's mashing together markup strings, I thought we were past that by now :-)

This all looks good to go to me. It'd be great to have @jannakha look too since they seemed to have success with the handler being set in defaultFieldSettings but I'll go with RTBC so we can move on.

This looks like a good enough solution to me, however there seems to have cropped up some nits not directly related to the change;

- The AJAX response contains an extra optgroup around the displays, this is only cosmetic but still might be worth looking at

- I saw a couple of JS errors from disable-form-elements.js where doesn't seem to take into account that an ajax instance in Drupal's list might be null. In my experience this happens when an ajax trigger has been updated. Just checking for the instance being null should be sufficient. I would say this should not be fixed in this MR since it really is unrelated.

Great that you're on the ball at least, @danflanagan8 :-)

I spent some time yesterday looking a the SelectionPluginManager to see how my adding 'handler' => 'default:view' to the (default) settings but not the (default) settings makes the problem go away and I really didn't get far.

The $base_plugin_id parameter to SelectionPluginManager->getPluginId() is sometimes NULL because the instance created by SelectionPluginManager->getInstance() is sometimes constructed with an $options array in getSelectionHandler (either an empty array or the contents of 'handler_settings' from the field definition) without a 'handler' key, which should be the field definition's 'handler' setting. The handler is obviously required, but somehow isn't always present.

It seems this happens when the SelectionPluginManager is called in the ValidReferenceConstraintValidator (phew, we're really Enterprising here)

In the working case (i.e. field settings page loads), 'handler' and 'handler_settings' are set

Drupal\field\Entity\FieldConfig
 ...
  settings => [
    "force_default" => false
    "allowed_views" => ...
    "allowed_display_types" => ...
    "handler" => "default:view"
    "handler_settings" => []
  ]
  ...

but in the non-working case (selecting a view from the dropdown) they are not

Drupal\field\Entity\FieldConfig
 ...
  settings => [
    "force_default" => 0
    "allowed_views" => ...
    "allowed_display_types" => ...
  ]
 ...

This comment turned out to be a bit of a stream-of-consciousness, but I wanted to share my poking around.

Whoops, meant to add a comment last week, but totally forgot about it.

With the last two commits to the MR, all of my tests pass in 9.5 and 10.2. Please review again.

I've made the change I suggested in the MR, but the upload test now fail on Drupal 9.5. I think there's an earlier version of FileIsImage getting in the way.

@dburiak You are correct, but I don't think we can use the DeprecationHelper here as (according to my quick check) it was introduced in 10.x and svg_image claims backwards compatibility to 9.3.

Just something simple like this should be sufficient, I think? It will cover the new case and fall back if the new way of doing things isn't being used. The FileIsImage change could even be stuffed in the same block, which alternatively could be a version check for clarity.

Opinions?

    if (isset($element['#upload_validators']['FileExtension'])) {
      $extensions = &$element['#upload_validators']['FileExtension']['extensions'];
    } else {
      $extensions = &$element['#upload_validators']['file_validate_extensions'][0],
    }

I bumped into this issue today and have put together a patch that might work better.

This is a partial refactor of how the module operates, I've (re)moved the hook_form_alter() and hook_form_submit() code into DraggableViewsField, utilizing the parent BulkForm methods of viewsFormSubmit (and viewsFormValidate) instead of doing that whole replace submit button business. My limited testing seems to indicate that it solves the interoperability problem with views entity form field without having to do any special casing for that module.

I won't put this to Needs Review or a MR as the tests have not been updated and it feels like I've just done a partial refactor that, while it does fix this particular issue, if committed would leave the module in a bit of a mixed up state.

The patch should apply against the production 2.1.3 release.

I might complete the refactor in another ticket, time permitting.

@quicksketch Apologies, my personal preference is tabs so my editor defaults to it. I didn't think of checking for it sneaking some in when I wasn't looking.

This issue finally bothered me enough so I have put together a MR that makes the module work again. Since i threw out the previous changes made in this issue, I've created a new issue branch. Please do review.

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

@vasike My apologies, I was just being dumb. Please ignore my comment.

I'm not sure if I've missed something obvious, but this change seems to cause views to pull in the refercenced entity with an inner join. This does not happen with the default NumericFilter where it becomes a left join. I had a cursory look at the code but couldn't find out where/why this happens, but perhaps someone more versed in views can have a look.

The patch in #265 applies but will crash the site, there's a missing use statement for the EntityDisplayMapper in config_translation.module.

I investigated the problem myself to look for a temporary patch, and it seems it was that the language_access module also overrides the ContentTranslationController. Adding 'Drupal\language_access\Controller\ContentTranslationController::overview' to the SoftTranslationRouteSubscriber made the "split off" entries appear and I could split the translation out.

Additionally while reading the code I also found out that mechanism for matching split off pages with their trnanslation counterpart seems to be solely using page aliases which makes this module useless to me.

sthomen → created an issue.

I ran into this as well on Drupal 10.1, it seems that the validation of the field somehow fouls up.

In this validation, SelectionPluginManager->getSelectionHandler() is called , and the second time the result of $field_definition->getSetting('hander') is null which causes the error with uasort in getInstance().

I patched this by setting the 'handler' key in ViewFieldItem::defaultStorageSettings(), it is probably the wrong thing to but will fix the crash for those in dire need.

Apologies for the MR mess, I have not used them before and it's a bit cryptic.

sthomen → created an issue.

This patch might work better. #11 requires that you have not already run 7300. Instead of removing the removal of the 'weight' field I just add it back in 7303. This patch (like #11) also removes the risky use of drupal_get_schema_unprocessed() (it will give you the _current_ schema, but how do you know it represents the expected schema at the given update?).

Something must have mangled the encoding for the patch in #23. When I fixed the cedilla'd c (and finally figured out how to run functional tests) in my local copy of the patch file, everything passed.

I've attached my fixed patch file, and hopefully it won't get mangled again.

I had this very problem on a site that was installed in english and the language later was removed; one user (but interestingly not user 1) could login and administer things just fine, but block role conditions would not trigger for them and in the profile editing UI the administrator role did not have a checkmark and the preferred language dropdown was blank (no languages at all).

I first tried seting the role using drush urol, which did not help, then I set their role using the drush php:cli and that did not help (but the value did seem to get updated in the entity), but changing the langcode field to an extant language made everything work again.

So my minimal fix tip; run drush php:cli, then

use Drupal\user\Entity\User;

$user = User::load(2); // the UID of the user you wish to fix
$user->langcode = 'fi'; // an existing language
$user->save();