🇧🇪Belgium @RandalV

Thanks for the fix, and sorry for the inconvenience :-(.

I'll release a new tag.

A small fix applied to new entities being saved, they shouldn't be processed in that presave hook.

Welp.. Sadly I have no idea how to fix that mess.

Instead, here's the patch as a file.

Hoo wee.. Something seems very wrong with this merge request, it targets the initial fork branch rather than the module's branch 🤦🏻‍♂️

I had to open a new branch because the existing one (1.x) created issues for me locally, due to it having the same name as the main module's git branch.

This attempts to make the functionality as 'global' as possible, by retrieving most stuff dynamically.
Please take a look and feel free to test it.

There is an edge case where this could "not work", which is when the route parameter in `entity.[entity_type].canonical` is not the same as the entity type machine name... But I can't think of a single instance where that would be the case.

I think this issue is definitely an important one, but the scope still seems too small.
I wonder why it should be limited to nodes and taxonomy terms at all, all content entities should be valid here.

I'll see if I can cough up a solution for that.

And another apparent oopsie, issues like this already exist 🤦🏻‍♂️

I'll link them and mark it duplicated, apologies.

Apologies, I forgot this has now been included in core and the issue queue has moved there.
Changing project + component.

randalv → created an issue.

Applied to 1.2.x, will come with when a new tag gets released.

randalv → created an issue.

Thank you for the fix! I've committed it and it'll be introduced in the next release.

Hi @rosie.catchpole,

Thanks for reporting the error, I'll take a look at the solution and will get back to you ASAP.

I had encountered a bug in a react application (embedded in Drupal on the same domain) that for anonymous users, no session id was present.
I found the functionality where this was set and extended it slightly, this should correctly set the session id if it's ever missing.

I also fixed the core version requirement I had mentioned before.

The tests are still needed though, leaving status as-is.

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

Hi Manuel,

You're right, I have not yet been involved with issues thus far :) I'll get into that ASAP.

I was hoping we could open a new branch for D10/11 compatibility (2.0.x), and immediately tackle all those issues.
That way, 1.x could remain 'supported' but not maintained for D9, and we can go forward in 2.0.x with modern code (OOP hooks and other newer technologies).

I'll keep you posted!

randalv → created an issue.

New tag has been released. Thanks again!

Hi @andybroomfield,

Good catch! The class name was altered at the last moment before merge, seems I forgot to alter the service name. Thank you.

I'll merge and release a new version.

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

Seems logical, I've added the hooks_converted parameter.

Since the other threads are resolved, I'll merge into 1.2.x

Processed the feedback.

I was also debating whether the Help and Cron hooks could be merged into a "Core.php" file since they're both small hooks, but I just kept them in their own file.
Figured it's better to keep things as split up as possible, which is the whole point of moving away from one gigantic .module file to OOP hooks :)

randalv → created an issue.

@svendecabooter the changes have been applied in the MR.
Feel free to check it out, everything seems to work fine.

Changed minimum Drupal Core version to 10.1, PHP remains at >= 8.1.

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

- Merged 3.x into the MR
- Changed the update hook so it lands at the end (I figure since this is a new functionality, logically we shouldn't let it run before newer update hooks)
- Added form states so the client secret is only visible and required if PKCE mode = off
- Added a deprecation warning explaining that not supplying the "$session"-parameter is deprecated, but doesn't break anything if that is the case

The MR applies again and seems to work functionally as it did before.

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

Hi julianriemersma,

Thanks for reporting the issue!
I've finally gotten around to provide a fix, I had to fix it a bit differently because removing 'authenticated' from disallowed roles was creating some other issues.

I added a new release.

Patch #2 works here as well.

As per this issue / comment: https://www.drupal.org/project/select2/issues/3474793#comment-15991481 🐛 Declaration of Select2::defaultConfiguration() must be compatible with FilterWidgetBase::defaultConfiguration() Active
Is this still active/relevant?

Merged latest changes from 9.0.x into MR branch, seems to apply and work fine on my end.
Setting back to 'Needs review' so someone else can have a test before it's merged!

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

Merge request added!

I also took the liberty to remove what I assume was an orphaned `var_dump` 😇

A little correction from my end, all the functions that call the "renderSiteSettings"-method in the extension class do seem to render with the correct language context, but the other two functions (`site_setting_field` && `site_setting_entity_by_name`) specifically check for a langcode and load that translation, so those should be altered I think.

I'll add a MR for it.

randalv → created an issue.

With the current MR changes, I run into a plethora of issues where the value is set to an empty array or null or something...
Not sure why this happens.

Causes issues on basically any entity form for me, for example when saving a new taxonomy term, then the 'weight' value is set to null and gives a WSOD.

Can confirm the issue persists and is solved by the patch, thanks!

Hi @julianriemersma,

Sorry for the delay, I'll try to take a look into this ASAP.
I think something else might be wrong still, it's normal the authenticated role can't be chosen (since you *need* that role to use Masquerade by Role), but it shouldn't deny access when trying to masquerade as authenticated.

I think I've found a solution but need to do more testing.
Will keep you updated.

Negate my proposed solution, I just saw it's a static function.
I tested it by adding a static property, but this doesn't seem to really help... The performance issue remains.

Perhaps a maintainer has a better proposal for this?

randalv → created an issue.

MR can be reviewed, it's a simple but logical change.

randalv → created an issue.

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

I was looking for this functionality as well.
We have a website that has pricelists 'linked' to a value on the user entity.

We import our pricelists from an ERP, and we've worked around this issue by hooking onto the user & pricelist insert/update events.

Pricelist on insert/update:
- Find all users with the custom value and attach them to the customers field
User on insert/update:
- Remove the user from pricelist(s) with the old value (if applicable) and add to pricelist(s) with the new value

The main problem here is that, if no users have the value of that pricelist, the pricelist applies to EVERY user. That's something that conditions would have easily solved.
In our use case, we've solved this by creating a 'dummy' user and if no other users apply for the pricelist, we attach the 'dummy' user to the pricelist.

Configuration/development wise this would have been much faster and cleaner if we had conditions, but I understand the argument of speed is a factor here too.
I'll set this as postponed, it's not and hasn't been really an active issue and if the need arises later on it can be reopened.

Ah.. my bad, you're absolutely right. I overlooked the default value of the class property.

I'm not sure if a backport to 1.x is still necessary, I suppose it depends on how long 8.x-1.x will remain supported? If it will, then maybe it would be good to have that fix there too 😇

Hi @mably, apologies, I hadn't seen that!

Although if I'm not mistaken that doesn't entirely fix the issue I was experiencing, because the encoder will still not strip tags or trim strings unless a setting is specifically provided (whereas before it would strip/trim by default).

With the fix currently in the 2.x branch, the warning message is gone, but if the encoder is used in Views Data Export it'll still let any and all HTML through (without the possibility of configuring it otherwise 😞).

It would be easily solved if encoders allowed configuration forms, but since (I think?) they don't, would it be preferred to keep the old functionality intact?

I've created an MR that adds default values. This preserves the behavior pre-#3108301.

randalv → created an issue.

Hi @mably and @dejan0,

Thanks for your solutions! I've tested the MR, and it seems to work fine for me.

randalv → created an issue.

I haven't got the time to fully debug this in facets itself, but in my case it was solvable by rendering it in a custom block like below.
I had to render in a custom block anyway because I wanted to add extra items before and after the exposed filters, so this was a quick fix for me.

/** @var \Drupal\views\Plugin\Block\ViewsExposedFilterBlock $plugin_block */
$plugin_block = $this->blockManager->createInstance('views_exposed_filter_block:events-overview');

// Make sure the facets are populated.
$view = $plugin_block->getViewExecutable();
$view->preExecute();
$view->execute();

return [
  // ...
  'filters' => $plugin_block->build(),
  // ...
];

I'm also experiencing this issue in a new project of ours using Facets 3.0.0-beta4, exactly as described above.

On the view page itself, everything is fine. Anywhere else, all the facets are empty :(

I'll see if I can find anything, haven't had the time yet to debug.

Drupal 11 compatibility is already implemented.

Hi @mitsuko,

I'm not entirely certain what the case was anymore but when I built this module, I ran into some strange behaviour when the authenticated role wasn't present.
I could test it again, but I figure it's not really important anymore.

In any case, the reason you didn't run into any such incidents is because with your patch, behind the scenes the module would still apply the authenticated role (even if you didn't select it) in the submit handler.

I think regarding the validation, we shouldn't worry about that anymore. Before it was deliberately made impossible to leave the role selectbox empty, so we had to double check in the validator to see if people didn't get around HTML validation and leave it empty anyway.
But since we now want it to be left empty when required, we don't need that extra validation anymore.

Any extra roles that are added through the inspector will also fail the form, since Drupal automatically handles that as invalid input as it wasn't in the form element's `#options` array.

It's been released in 1.0.18.
Thanks for your suggestion!

Hi @mitsuko,

I see your point, it would be useful to only masquerade as the authenticated role, however I think I'll fix it somewhat differently.

The authenticated role *should* always be selected, so removing it from the automatically selected roles might produce strange results.
I'll just remove the 'required' parameter from the roles selector, leaving the field empty will result to only the authenticated role being enabled.

That should cover every use case.

I'll reopen this since the fix hasn't actually been released yet, nearly 2 years later.

@Maintainer, could you please release a new tagged version with this fix present? Thanks!

I can confirm the MR applies to 1.25.

I now get the aria-label="Cookie privacy banner" parameter in the popup's HTML tag.

(PS. Let's keep working in the MR -if anything needs to change- rather than attaching patches)

Please, feel free to review.

This fixes the issue of terms always being rendered in their default language, rather than the current language.

randalv → created an issue.

Thanks for the patch/MR.
I've applied the MR diff to one of our sites, and the issue has been resolved.

Hopefully this finds its way into a release 🙏

That's a very interesting development... an array containing the serialized content with `50` as its key, thanks for sharing @mahde!
I'll debug some more too and see if I can reproduce it somehow.
I also notice it adds both roles twice, interesting!

Hi @mahde,

Thanks for taking the time to debug and updating the MR!
This brings us back to my comment in #5 🐛 TypeError_: unserialize(): Argument #1 ($data) must be of type string, array given in _unserialize() Postponed: needs info though, there's only one piece of code in the module that updates the user data for roles, and it always serializes it..
I don't understand how it could possibly contain an array unless some other module messes with it 😓

I'm sorry guys, I've tried debugging this but nothing seems to point to an error 😓

Could you perhaps provide me with a list of contrib modules currently in use?
That way I could test with those, maybe one of them triggers something malicious in msqrole.

PS. I did fix some little bits here and there in the latest release, in the D11 compatible version there were some regressions, but none of which should have caused the main issue raised here.
The "access denied" error you mentioned, @mahde, should be fixed though.

Hi @mahde,

If you could also please provide more information on how to reproduce this bug, we could move forward with this.
Currently on a fresh install of D10/11, nothing of the sort seems to happen..

Thank you!

Hi markconroy,

Thanks for reporting a possible bug.
Could you provide a little more information on how to reproduce this error? I don't seem to get this on a clean install.
Any contrib modules that might interfere?

From what I can tell, the unserialize function in the below piece of code should only be run if the data isn't empty.

    if (!empty($this->getData($uid, 'roles'))) {
      $data = unserialize($this->getData($uid, 'roles'));
    }

However, it should be impossible for this data to be anything but a string, since it's only set by one function (the below code) and that code serializes the value before committing it to the DB.

  public function setRoles($uid, array $roles) {
    return $this->setData($uid, 'roles', serialize($roles));
  }

Hence why it sounds like some external source might be messing with the msqrole user data 🤔

PS. While the MR seems just fine, it simply shouldn't be needed.. Hence why I currently see that check more as unnecessary clutter, so I'd prefer to be able to reproduce it and perhaps fix it in a different way 😇

Kind request for the current maintainer of this module to grant this person co-maintainership.
I see there's no longer any supported version, while this is definitely a useful feature.

Thanks in advance!

@ozin MR updated according to the last patch provided by @Dylan Donkersgoed.

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

Hi @remoschneider,

Please, refrain from posting a patch that isn't functionally brought up to date.
The patch you posted applies, but is buggy due to the lack of the 'getPluginBlockId'-method in the InlineBlockEntityOperations class.

I'll post a patch that is functionally in line with the old one, if someone wants to turn it into an MR.

@alexpott

* MR applies cleanly to 10.3.x
* Done some tests and the results are as expected:
* Same hash salt -> same session cookie name
* Different hash salt -> different session cookie name
* Can stay logged in across all subsites of a multisite on the same hostname with different hash salt
* Giving all subsites the same hash salt has the expected behavior of being logged out when visiting a different subsite

Seems ready to me! Thanks for the MR.

Rerolled #41 against 10.3.x

Hi @google01,

Merge request definitely applies to 10.3.x.

The contents of this: https://git.drupalcode.org/project/drupal/-/merge_requests/5373.diff
Should be downloaded and placed in a local folder (e.g. YOUR_PROJECT/patches/core/3091336.patch), make sure there's an empty line at the end, and then add `patches/core/3091336.patch` in your composer patches list.

Could a maintainer please take a look at this issue?
It doesn't change the module's initial installation state, and it adds a lot of possible functionality.

I think this could really spearhead this module's already superb usefulness.

Is there any progress on this, or any way to circumvent this and move forward?
Recipes are included in 10.3.x, I feel like the priority of solving this should be a little higher now.

I can't seem to get it fixed myself, other than patching core and removing the functionality entirely, but that seems a little nuclear...

On the other hand, I personally do think that this copy functionality should be removed.
It definitely doesn't make much sense to copy blocks from one theme to another, given that regions could be entirely different.

Thank you for your patch, alecsmrekar, it seems to work.

It seems to work, but I am however unclear on why this issue pops up at all. why is the `layout_builder.move_block_form`-route being triggered when the arguments clearly call for the `layout_builder.move_block` route.

I'm wondering if changing the path for one of the two routes would be a more stable solution?
Two routes with very similar paths and arguments, that seems like asking for trouble to me.

Perhaps all non-publicly accessible routes (like ajax routes) should get some prefix, for example.

Thank you for your fix, Anmol. It's been committed and released.

The fix looks good at first glance, but please leave issues in needs review until the maintainer has actually committed the changes.
I'll try to do this ASAP.

RandalV → created an issue.

Merge request added.

RandalV → created an issue.

The problem seems to be that in file core/modules/field_ui/field_ui.module,
The $form['field_storage']['subform'] doesn't seem to exist, in the function below.
I don't know what the correct solution would be.. I'd say the form still needs to be alterable, and we still need to trigger the alter hooks on the bottom.

/**
 * Implements hook_form_FORM_ID_alter() for field_config_edit_form.
 */
function field_ui_form_field_config_edit_form_alter(&$form, FormStateInterface $form_state) {
  if (!isset($form['field_storage']['subform'])) {
    return;
  }

  $field_config = $form_state->getFormObject()->getEntity();
  assert($field_config instanceof FieldConfigInterface);

  $form_id = 'field_storage_config_edit_form';
  $hook = 'form_' . $form_id;

  $field_storage_form = \Drupal::entityTypeManager()->getFormObject('field_storage_config', $form_state->getFormObject()->getOperation());
  $field_storage_form->setEntity($field_config->getFieldStorageDefinition());
  $subform_state = SubformState::createForSubform($form['field_storage']['subform'], $form, $form_state, $field_storage_form);

  \Drupal::moduleHandler()->alterDeprecated('Use hook_form_field_config_edit_form_alter() instead. See https://www.drupal.org/node/3386675.', $hook, $form['field_storage']['subform'], $subform_state, $form_id);
  \Drupal::theme()->alter($hook, $form['field_storage']['subform'], $subform_state, $form_id);
}

Hi frederiko_,

Certainly!
Editing the "body"-field will lead you to a URL like:
/admin/structure/types/manage/page/fields/node.page.body

Replace the last part (the field's machine name, in this case 'body'), with the locked field's machine name, for example:
/admin/structure/types/manage/page/fields/node.page.layout_builder__layout

And it'll give you the WSOD.

RandalV → created an issue.

Thanks to #9 and #16, I can confirm that adding the "wrap"-item even as the last item on the bar is a solid workaround until a permanent fix can be provided.
It wraps the excessive items neatly on a second row (as was the behaviour in CKEditor4 if I'm not mistaken).

I'm closing this issue, it's been committed to the next release and it seems to work fine.

It would be nice if this could be committed.

I'm unable to upgrade right now due to this issue, I'll take the module out of composer and put it in custom for the time being until this is released.

In general, the composer.json file seems to lack behind the info.yml file?
In the info.yml file, drupal 10 is supported.. But in composer.json, it's not.

I added a fix for the non-scalar value problem that relates to this issue.

I can't seem to create a merge request from the issue fork though... Hopefully you can?

Can confirm I also still get this, I'm using the google_tag module and in Context (any) the value was: *

Simply removing * and leaving it empty works for me, but that won't work for anyone using a value like: generic_name_*

Latest commit moves the responsibility of escaping the search result labels to the controller, rather than every plugin having to do it separately.
This also ensures any results added through the (perhaps deprecated after introducing the plugin type?) coffee_commands-hook.

Thank you for reporting and fixing the issue 🙏🏼 🙏🏼

I pushed it, and released 1.0.14 containing this fix.

I altered the default config so it's identical to the current situation for the end user.
Extra plugins (content + people) can be enabled and configured afterwards.