🇳🇱Netherlands @roderik

Actually, this still happens on my D10.3 too.

So the status is still what it was per #12 and I'm setting this back to active.

By now, I should probably re-check

• whether I can isolate the configuration that is causing this, and describe it better, and make this into a 'fail test case'
• whether the MR in #3416934 solves this. (Unfortunately, I found this validation code quite hard to make sense of, last time I tried.)

So: reopening, setting to active, and leaving the "Needs steps to reproduce" tag for now. I hope I'll get to it soonish.

Just for reference: this was never resolved.

The status page still crashes on D10.2.

I don't know what happened with the issue branch in 🐛 Config inspector stopped working after Drupal 10.2 update Active : whether it was just forgotten, or maybe not applicable for some reason.

Either way: it doesn't matter anymore, since we all moved on to D10.3+ now.

Oh wow, this was apparently reported ~2 years ago...

Added a similar report to the same issue (I came here to report wrong dates, and saw this one)

roderik → created an issue.

[ catching simple reviews from fago ]

TIL trait names are case insensitive (unlike class names). So even if someone else would already be using this trait, things will not break.

(Tested by only renaming the trait, and leaving the 'use' statements unchanged: things still work.)

Discussed in person, IMHO no need for me to reiterate here at this moment

... I don't want to argue naming or structure of the (currently) single test file in user_login_form, here.

I agree that login (i.e. the POST request) is already tested in LupusSessionDomainFunctionalTest, though it's done implicitly as part of some other tests.

So I guess this covers what is needed / addresses earlier feedback.

Duplicate of #3168635: Notice: Undefined index in views_handler_filter->accept_exposed_input() → . Not exactly the same solution, but released with 7.x-3.25 (quite some time ago) and the comment without the $value isn't necessary to continue in the method. left on that issue, indicates it does the same thing.

It just doesn't log a warning about a possibly-broken handler.

So the only thing this MR does now, is also check

• the old authmap_uid value, in case some magic is still caching that for people
• the users_field_data_authmap_uid value, which has always worked and maybe we should just only check this value, but I would rather play it safe / not think about how it is constructed adn what might cause it to change in the future...

And @vladimiraus: if returning NULL causes an issue, then IMHO we need more concrete info about what code has/causes the issue, before we fix it.

In the meantime, I'll commit and push the same construct to samlauth in 🐛 Error encountered when accessing SAML authentication mapping configuration page Active .

Commenting here instead of in MR, consolidating thoughts in one message.

1. Per my last message, I have no clue how people are still seeing errors. (Some existing view-configuration remaining in the site? I also don't know how the samlauth-specific view would influence these reports.) However, given that we still have reports, and an MR, I guess let's go with that MR logic.

   But yes, the provider parameter should be reinstated. Seems to be too quickly copy-pasted from a commit to 🐛 Error encountered when accessing SAML authentication mapping configuration page Active .

2. Per #14:
   

   parent doesn't cater for NULL

   I don't see parent not catering for NULL. LinkBase::getUrlInfo() is an abstract function without a return value definition in code, whose comment says @return \Drupal\Core\Url|null. Also, the only (code somewhere in a) caller I see handling the return value, as $this->options['alter']['url'], handles a NULL value perfectly. It just renders "delete" as non-link text, which is expected in that case, IMHO.

3. Indeed we should not be throwing an exception while building this view. But given the previous point, I think the simplest is to just keep returning NULL. If some other code isn't handling a NULL value for 'url', then (according to the return value documentation) that's the point that should be fixed.
4. I started this whole mess by not providing a 'base' in externalauth_views_data(). Sorry for that.

Will try to fix the MR.

Just some details/rambling if I ever want to read myself back:

• I now checked. It's clear from code in View::isInstallable(), that a 'base' should always be defined for the table. So apparently I did not test uninstalling and reinstalling the module, when I provided the initial MR :-(
• I don't know 100% what this 'base' does, or whether a 'field' value inside it is super necessary. But I'm not going to mess with that now.
• In my initial MR, I was hesitant to add the uid field as 'base', because uid is not a unique field. This also means that if we add a 'join' to the authmap table from a user table, we might get duplicate records in the view. However... I'm guessing (?) that the ability to add this possibly-problematic join is not related to the 'base' info - but only to the 'join' info on the table, which is already there (since 2018). So... shrug. Not thinking about it further.

This (setting auto redirect, but then allowing a passthrough by way of a token) looks like quite a niche thing to me.

Unless I hear more about this, I think this is better off in your own custom code.

You can make your own event subscriber that subscribes to KernelEvents::REQUEST, with a higher priority than RedirectUserLoginFormSubscriber. That also frees you up to use a non-cacheable response, while RedirectUserLoginFormSubscriber can still use a cacheable one.

Per June 2023, the docs are now copied from node/425990 to https://www.drupal.org/docs/getting-started/installing-drupal/using-a-lo... → and extended a bit. I think they are fine.

I'm still not 100% sure how the original issue came to be (why is HTTPS 'on' while the port is not 443) but until further notice I'm still guessing that the solution is in that documentation.

Re-categorizing to "Support request" to fit all the other linked issues.

Can you tell me more exactly what your situation is and what you are trying to achieve?

From just reading the patch, it looks like

• in Drupal, you have a Single Sign-on Service configured, that contains a TargetResource query string, e.g. https://example.com/path-to-idp?TargetResource=SOME-URL
• Your code makes sure the TargetResource has a trailing slash, i.e. https://example.com/path-to-idp?TargetResource=SOME-URL/

And that's all it does?

In that case, why don't you configure the Single Sign-on Service URL to already have that trailing URL?

In the case that $certs is not set, there should be a conditional check beforehand, to avoid PHP warning messages.

I don't think so: $certs can never be "not set"; if the config value is not set, $certs becomes an empty array. See the line just above what you patched:

$certs = $config->get('idp_certs') ?? [];

It could be that in your case, the 'idp_certs' value is set to an empty string or other empty scalar value.

But that really is a misconfiguration on your side. 'idp_certs' has never been a scalar value. And especially now that we have config validation, we can expect this to be a valid value. Besides, it's only a PHP warning, not a fatal error.

So the solution is just to fix your configuration values.

samltest.id is now offline so I guess this issue can be closed.

The link to https://symfony.com/doc/current/deployment/proxies.html is now added into the text of https://www.drupal.org/docs/getting-started/installing-drupal/using-a-lo... → which someone copied from the older page (and which is now mentioned in the README).

I think that's enough of a (indirect) reference to the possible solution to un-set "Needs work". Keeping this open like many other support requests in this queue, for people to browse through. (Maybe I'll organize them... some day...)

I've created the unit tests now, which can serve as a kind of reminder/reference on how the login/user-save procedure works.

This reminded me that in 'plain' Drupal Core, the email address is actually not unique. Logging in a second user that saves that user with the same e-mail address, is (in some circumstances) possible.

I'm not 100% sure, but the database error seems specific to CiviCRM to me.

Now... I can imagine that someone wants to mark email (or any other mapped field for that matter, e.g. an employee number) as "unique", so the samlauth module always throws an error instead of trying to save the second user. Until now, this is only done for the username field (which is truly unique on a database level).

That would be a valid feature request and I'll keep the issue open for other people to stumble across / comment on. But I'm not planning to work on it myself.

Thank you for reporting. I think it's time to remove those references in favour of references to a Docker based IdP, and have just added a general "you can use an online service" remark without specific instructions, because I personally don't use those.

Thank you. Took a while, but now added to samlauth module.

Thanks.

End of year, finally getting to volunteer modules.

I guess this isn't implement in the PHP-SAML module because it needs a cache backend.

I guess what this needs is a cache backend to keep all IDs(?) of OneTimeUse, for a configurable period of time. It should probably be a "normal" cache bin so site administrators can make the cache permanent if they want by e.g. installing + configuring pcb → .

I was wondering why this doesn't get more attention on the internet than it does. Probably that's because, if HTTPS is used for communication with the IdP and the IdP is configured to common standards (i.e. only answer to requests from known certificates), it isn't that easy to steal SAML requests / assertions.
(Maybe on a public computer that has a special browser extension installed to snoop and save those assertions...)

#16 is not related to the rest of this issue, so I overlooked it and its reasoning.

A similar thing has been committed in 💬 Flag for disabling locally Active .

I'm going to set "fixed", among others for attribution to kick in. Feel free to reopen with extra info / response.

===

Ah, that pesky hook_user_presave. Thanks for pointing me to it. I see it interferes with new user creation.

Fixed now. I added code that should have the same effect as #2925171-16 but does not need an extra flag.

For the rest, I'm still not convinced that we need a new flag for "disabling locally". All other reported issues have to do with restricting local login... which can all be unset. And I put some drush commands in the README (see the above commit).

@jwilson3:

1.- On LOCAL, I cannot use drush uli to login as existing users, because the Drupal login functionality is disabled in favor of University SSO for all user roles. I dont see an option for login auto-redirect nor is the anonymous user listed as a checkbox in the UI for allowed roles.

I don't know why drush uli isn't working. It works for me locally Unless my memory is failing me: we are not restricting drush uli because "a user who gained drush access already can do pretty much anything anyway".

There is now an option for auto-redirecting /user/login to SAML → , which wasn't there yet when you wrote your comment.

So both cases look like custom code additions. A module flag can't override that custom code, you'll need to get people to change the custom code. (If they are looking for some module config option to use for the basis to decide whether to restrict login: I suggest either of the options we're discussing/documenting here.)

You don't need to have the "Anonymous" role listed as a checkbox in the UI for allowed roles. What you are looking for is the "Authenticated" role, which every user has. Check that, and every user is allowed to log in.

4.- It is also less than ideal to have to go to the module configuration screens to change the roles that are allowed to use local Drupal login, this university site has 30 roles.

5.- Both options 3 and 4 are cumbersome to do manually and hard to document for other devs.

Not dependent on the number of roles. Just check "Authenticated" and you're good to go. I included the drush command in the README, which seems pretty easy to document: drush config:set --input-format=yaml samlauth.authentication drupal_login_roles '["authenticated"]'.

1)

Thanks for your opinion / the MR. I think it's good that this is implemented by someone who is using the feature.

Given that the SAML spec only supports specific contactPerson and organization attributes, and that the php-saml library further only supports certain values within those attributes

That's true. I think it's remarkable that php-saml only outputs GivenName and not e.g. SurName, but who am I to second guess it...

Also I see there's more than technical and support ContactType, but I'm not going to second guess your implicit assertion that (only) these are the best to implement.

There seems to be a bit of redundant info there though, and I have a small question in the MR too.

2)

I'm not sure about the "attributeConsumingService(s)" thing and whether it belongs here.

See commit; the check just needed to happen before the linking, instead of after (and tweaked a tiny bit). Which makes sense, but I didn't see that beforehand.

Anyway: happy that this is now tested, so we'll spot it if it breaks.

roderik → created an issue.

Category 2 tests are actually done, as part of 🐛 Linking a local user account to a saml account may be incorrectly blocked Active . That makes me feel a lot easier about the module.

(The code is still spaghetti, but it turns out Kernel tests can live with that / calling current SamlService::doLogin() actually works even with our hook_user_presave implementation and the other things it calls. Yay.)

I still get caught by the fact that the pre-formatted commit message cannot be edited, when merging from the drupal.org issue. Otherwise I'd have changed it slightly.

Anyway, fixed and credited.

• The result was good, but you somehow rebased (copied) the ~39 newer commits from the 8.x-3.x branch onto the existing change, before merging.
• In this case, a single merge commit is better (when working on a branch with others).
• You could also better to rebase the single change commit onto the newest 8.x-3.x... if you are allowed to, and feel confident enough to force push (overwriting the original commit). I do that often IF I'm working alone on a branch.

In this case, I rebased and force-pushed into the branch (to have cleaner history)... because I was going to merge it and close the issue anyway.

Anyway, that's detail :-)

---

Thank you for the fix. This is a clear bug, but I had ignored the issue for the previous release. Among others, because I felt like this really needs tests, to make sure that it doesn't break anything else unexpectedly. (The code in the fix looks fine, but... the existence of the bug already proves that things are getting too complicated to not have tests. I guess I should have set the issue to Needs Work in the meantime; it took longer than I thought.)

That's done now. Tests for the user creation/linking logic are included, which makes me feel a lot safer for future rewrites/fixes. I also discovered another bug, which I'll fix in a related issue, and added some tiny code tweaks and things to the README while thinking about tested behavior.

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

(Oops, forgot names in commit message. But credited while fixing.)

Thanks @alesharries. Again, dumb mistake when refactoring the form.

The unrelated bit is from 🐛 Bug and confusing behaviour when trying to set a custom NameID format which is already known to the module Active and has been fixed in there / should have been pushed to that PR, instead of this one.

Thank you for fixing this. The fix is exactly as it should be. 🐛 Redirect after login not correct when using base_path Fixed should never have been committed as-is, especially since the main maintainer of this module (me) is probably the one making the most noise about these errors, and possibly the only one spamming people's logs.

(
By the way, I'm not even sure that the warning is accurate anymore, since 📌 Exception in EarlyRenderingControllerWrapperSubscriber is a DX nightmare, remove it Needs work has been unceremoniously committed, after waiting in limbo for 8 years. I'll have to read up on the current situation / re-test which exact circumstances would still make Core trigger such a "leaked cacheability metadata" exception.
)

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

OMG that was dumb. Thank you for fixing.

Unfortunately I wasted your time and mine, because:

• I assumed I needed to review the MR. But you had not pushed the patch from #8 to the MR. So I spent time trying to understand the current MR when in fact the fix is a single llne.
• Then I saw that parent::submitForm() is completely changed (has had new 'save' code added) in D10.3 and tried to understand what had happened in ✨ Add optional validation constraint support to ConfigFormBase Fixed , 📌 Allow all callables in ConfigTarget Fixed and more. When in fact this had nothing to do with the behavior; I thought parent::submitForm() was re-saving the value but it isn't. It is my own code (until a single line is removed).

I doubted about adding more of your MR, but didn't. If you add 'urn:oasis:names:tc:SAML:2.0:nameid-format:transient' as a "custom" format, the form recognizes it and displays just "Transient" -- which now should hopefully be obvious enough.

OK. Last time I looked, I only saw the changes to views classes in 📌 Support for Search Index View and Rendered entity row Active . Now I see extra stuff there. So it makes sense to review that first and leave the "fields" part from this issue, out of it, yes.

But then, if I understand correctly, the title and summary of this issue will already be solved by #3493780. Which is great; that is (now) tested.

Since all the non-fields parts already have tests, I think the addition of another test for the 'fields' part would likely not be that much work? But we can quibble about those details later.

Beidres the comment on the MR:

The changes to CustomElementsPage.php and CustomElements.php seem good, and I'm confident they are well enough tested by the tests included. They can be committed.

However, the whole lupus_decoupled_search_api module... contains zero code. It's only dependencies on lupus_decoupled_views + search_api_db, and a README.

• Isn't this better off being a recipe, instead of introducing a new 'empty' module?
• Then we can move tests/modules and tests/src in the 'module root' tests/ folder. I see no issue with that.

Needs work for answering that + the above MR comment.

Note I haven't reviewed point 2 of the proposed resolution yet.

> step 2, change the class implementation of KeyPair to replace phpseclib with base PHP OpenSSL calls

Ah. That makes a lot of sense (and cancels the need for all the things I just made up on the spot while reviewing).

We discussed this in person.

The original intent for creating this ticket, was a "HTML to CE parser", that should not be inside a CE Field Formatter. It should be a helper class/service. (Unless it turns out to be so easy that no parser is even needed.)

Naming: either CustomElementHtmlParser or CustomElementUnserializer, I guess. (Does not need an interface; CustomElementGenerator also is a service without interface.)

This came out of a strangely-specified internal ticket. Just parsing the HTML to CE and outputting it as JSON unchanged is not a good use case for a Field Formatter. (It is very well possible that we will create a Field Fromatter later, once we do something with the parsed CE structure and actually change it. But that's a broader scope for this ticket.

How to continue:

After my interactive review, we realized that I don't even know enough about slots etc. to be of good help in reviewing all details. E.g. I have questions about how this example would be represented in a Custom Elements structure:

<p>There is text before <em>an emphasized string</em> and also after it.</p>

...and whether to use addSlot() or something else.

So:

Likely a good use of your time is to first do 📌 Create test for checking markup serialization Active , which will make you work with the CustomElement class and which is very necessary.

After you implement the "CE with attributes, no children", you will have a decent structure for the test and I can assist if you have any questions about slots (which I do not currently have answers to).

We can get back to this ticket after that is done. Then we can

• decide whether a separate 'unserializer' is worth it, or super simple / where to implement it
• add tests for it. (Various examples, unserialize and then re-serialize to HTML markup and see if it is still the same? Or?)
• decide whether to move on with adding a Field Formatter (likely in a separate followup issue) for specific functionality, or whether that is going to stay 'custom' code.

A Field

That looks good. But

1) I see that there are a few differences between this and 📌 Support for Search Index View and Rendered entity row Active code, while they are solving the same thing.

The code in this MR contains one more separate path than the #3493780 MR, because this has if ($value->options['type'] === 'entity_reference_entity_view') {

The #3493780 MR contains some more code abstractions and updates to new syntax.

This MR has phpcs errors which are new. (I think phpstan errors are preexisting...)

I assume we are going to merge this MR first? I don't have an opinion on whether the " code abstractions and updates to new syntax" should be included here, do whatever you want in that respect. As long as phpcs is solved.

2) This needs tests. I was a bit hesitant whether I would want to set this ticket to "Needs work" for that, but @fago in #3493780 commented the same, and I agree with him.

This is sufficiently abstract that we should make sure it keeps working. Add an example view (if not there yet) for every case that could produce different output / make different calls (3 or 4 cases?) and just test that some output still looks sane.

Seeing that graphicsmagick convert works the same way, so I added the same change to buildGraphicsmagickConvertCommand().

I only tested manually on Imagemagick 6, that this fixes our issue.

roderik → created an issue.

roderik → created an issue.

The proposed fix / MR does not provide a fatal error; it just shows a warning, but it still allows the user to save the form unmodified (so does not force correcting this mistake before saving).

This happens to be the easiest, because I couldn't make form errors to work immediately. (I don't know if that's because of the table structure, tabledrag.js or whatever.)

But also, I like it this way. If you have other things to change in this form, it's still possible to do those without affecting the missing formatter IDs, and fix things later (by e.g. enabling a missing module).

roderik → created an issue.

Rebased (because Github complains the branch is un-mergeable for no reason), and added 1 commit which updates to our latest conventions, now that 📌 Doublecheck CustomElementsFieldFormatterBase::getSetting() Active is committed.

Some summary of that issue's outcome and our code conventions that I will stick to:

• We use $this->configuration[KEY] instead of $this->getSetting(KEY) in our code. (They are interchangeable; we are apparently acting as if getSetting() does not exist, because we would rather it didn't exist. But it's part of an interface we implement, so we can't remove it.)
• never implement defaultSetttings() (which we also would rather see disappear); always implement defaultConfiguration().
• KEY does not need to be checked for existence, as long as it's defined in defaultConfiguration() + we are implementing CustomElementsFieldFormatterBase, which merges the defaults into $this->configuration

In the meantime, FileCeFieldFormatter got similar additions in ✨ Create a image-style dropdown to the file/image formatter Fixed which also needs to be updated according to the same conventions. Doing that in the same commit.

Also:

I was wondering about 'optional' configuration values. My feeling says that I don't like them always being present, in saved configuration. (Why would we save e.g. custom_format: "" when we never set or use it?). However,

• It is easier for the code, to define it in defaultConfiguration()
• I tested Core field formatters, and they (at least TimestampFormatter) also always export all configuration settings, including optional/empty ones. So let's treat that as convention, then.

roderik → created an issue.

For reference to whomever:

Markup:
See renderCustomElement() in existing tests.

JSON:
$this->getCustomElementNormalizer()->normalize($custom_element). (There is no place in custom_elements module that does JSON serialization, but the building blocks - i.e. getCustomElementNormalizer() are here so the test should be here. One module that has the above line of code, is lupus_ce_renderer.)

Considered outdated: the code that was eventually committed was simplified (relative to when this issue was created), and it's a private method now.

and replace the contents of key_asymmetric_get_key_properties() by something that works.

Per #3489430-4: Create a KeyPair service to handle general key management → , this is now changing to "and implement your own service that does the same as this one -- and optionally do some extra work to generalize things (see the comment).

Needs Work for at least the proposed class name change. If you don't agree, you can merge.

This looks good. One naming nit in MR.

At this point, if people do not want to be dependent on phpseclib, they need to overwrite the key_asymmetric.key_pair service with their own, and they need to extend the KeyPair class because there is no interface yet.

Two separate areas of future extension:

1) so that other services don't need to extend KeyPair:

• define an interface
• make KeyPair implement the interface
• mention the interface (not KeyPair) in the constructor method signatures

2) If we want this to be code-free, i..e people just install another similar service that uses another library, and it (usually) gets picked up automatically, we need to make the current KeyPair service just one of potentially many, and

• give each service a applies(): bool method (our implementation being e.g. {return class_exists('phpseclib3\Crypt\PublicKeyLoader';}
• add an extra service (e.g. KeyPairManager) which is a service collector → and also implements
  

  getKeyProperties() {
    foreach ($this->sortedKeypairProviders as $provider) {
      if ($provider->applies()) {
         return $provider->getKeyProperties();
      }
    }
    return some-default;
  }
  

• tag the current KeyPair service to be such a provider
• use the new KeyPairManager whereever we now use KeyPair, in our code.

Those can be later extensions, when this actually gets implemented by another service / needs to be swappable. Nothing in this issue stands in the way of it.

roderik → created an issue.

@useernamee we discussed this elsewhere but I can't find it. Summarizing just for reference:

• true, I don't think we need the config_rewrite module at all.
• Just realizing this now: we can and should remove config_rewrite from the composer.json because lupus_decoupled_menu is the only thing using it.
• About getting rid of lupus_decoupled_menu completely: that may be well possible -- there are things in the .install file that seem like they are either unnecessary or can be moved into a recipe in the future -- but let's create a separate ticket for that if needed.

Current status of this ticket:

I think we all agree this is the way forward. However, our internal integration tests indicate that somehow,

• the API response for menu items was being cached (2nd request is a cache hit)
• with this change, it somehow isn't anymore.

It would be good to look into the reasons for that, so we're sure to know all implications. This ticket is blocked on that - which may take a little while.
(And also the MR comment.)

Disregarding the fact that we need to have a MR instead of a patch, here:

We looked at this together and I agree that there seems to be no need for config_rewrite to be used (anymore) by this module, and it's just in the way (because it is rewriting new settings that the rest_menu_items has added, to NULL):

• It was added in 📌 Adopt config_rewrite for install time config Fixed == https://git.drupalcode.org/project/lupus_decoupled/-/merge_requests/43/d....
• The rest_menu_items.config.yml values are (and were) exactly the same as in the rest_menu_items module, so config-rewrite is not needed for that.
• The #3340799 MR also added a rest.resource.rest_menu_item.yml (but in config/install, not config/rewrite). I am guessing that the addition of config-rewrite might have something to do with that... but... that file is gone now. We don't need to support it anymore.

I added to the bottom of my previous comment, saying "However, 2. is not perfect for many use cases, either. "

I cannot follow, could you elaborate what changes?

Situaition: on a certain bundle, you have

• entity view displays enabled for 'default' and 'full' view modes;
• no CE displays

In this situation, custom elements in 'full' view mode will be bult based on the 'full' entity view display. (CustomElementGenerator::getEntityCeDisplay() will auto-create a CE display that is based on 'full'.)

This IMHO is the only logical behavior, and it's v2-compatible.

However,

1. this fact is not visible in the UI. You only see the one "default" (auto-created) CE display, and no tab/local tasks for "Full" etc.
2. as soon as you save the default CE display, this behaviour changes. Custom elements in 'full' view mode will be bult based on the default CE display. This also IMHO is the only logical behaviour.

I am just thinking about this now...

1. can be fixed by showing CE displays in the UI (auto-created if they don't exist), for _all_ view modes that have an entity view display... when the default CE display is nonexistent / disabled. This likely only needs a change to the RouteSubscriber, to also show routes to $path/ce-display/{view_mode_name} if no 'default' CE display exists and the "$view_mode" entity view display exists.
2. can be fixed (only in the UI form) by, if a default CE display is saved for the first time, at the same time also saving those auto-created displays. Then the behaviour doesn't change for those view modes.

That's a whole lot of 'invisible' behavior, so I wouldn't implement that without your agreement... but it feels doable, without introducing extra problems. (Just added the code complication, but that's contained to the UI form.)

Related: 📌 Fix confusion in UI when no CE display exists Active gives info / warning message about what is happening when no CE displays exist for a bundle.

Tweaked message a bit - this is now what is showing on a fresh Core install for articles.

Thank you. the issue been committed.

I added a different patch than #3:

• basic_auth_path_matches_current() is replaced by a function that resolves to the menu path, using menu items instead of the path.
• the 'return TRUE' just below this line has been replaced by a menu access check that occurred later in the code, because... basically that seemed better. (Though the first bullet point on its own is the significant thing.)

I discussed the patch outside of this queue, because - well, please see the release notes → .

Then,

• in the process, I somehow was made maintainer of this module.
• unfortunately there was a three year delay / I forgot about things.

To repeat my previous message: patch #3 can/should be discussed in #3063585: path is not registered in system. → instead.

mr.baileys → credited roderik → .

I've tested the update hook well, while updating config from our internal project.

I've now also tested several integration test runs (i.e. browser tests etc) with this changed code in place.

@stephenplatz I'm looking at Core now rather than just replying immediately:

line 89 could change to
$changed_role_ids = $account_role_ids = $account->getRoles(TRUE);

However... I'm not sure if the code is actually wrong for usual cases, if I look into the definition of User:;getRoles() and User::addRole(). then what I am reading (and I'm not trying it at the moment), feels to me like:

$account->getRoles()  // returns [ 'anonymous' ]
$account->get('roles')  // SHOULD return []
$account->addRole('member');
$account->getRoles()  // SHOULD return [ 'member' ]
$account->get('roles') // SHOULD return [ 'member' ]

... and I'd check, just in case, if I am saying something wrong here and/or if your database / saved configuration contains any role mappings to role 'anonymous'. It should not.

If I am right, then I guess

1)
$changed_role_ids = $account_role_ids = $account->getRoles(TRUE); isn't actually good for the module, because it does nothing except introduce a possibility to add "anonymous" (which throws an exception)

2)
your suggestion for a change is good for always calling $account->removeRole('anonymous'). which doesn't throw an exception, but I'm not super sure I'm happy with that as-is, because it will re-save the account on every login.

yeah, I think we were cross-posting while responding to ourselves :-)

oooooh.... now I understand.

Removing the "anonymous" role.

I guess my mistake here is assuming that $account->getRoles() is actually representative of the roles saved in the database. When "anonymous" is returned when the actual roles are zero.

Yes, that would mean that the fix is


if ( array_diff($changed_role_ids, $account_role_ids) )
{
if (getRoles() contains "anonymous" (which is equivalent to == ["anonymous"] , because it never occurs together with another role ) )
{ remove "anonymous" }

foreach (array_diff($changed_role_ids, $account_role_ids) as $role_id) {
$account->addRole($role_id);
}
}

Lines from 220:

      foreach (array_diff($account_role_ids, $changed_role_ids) as $role_id) {
        $account->removeRole($role_id);
      }
      foreach (array_diff($changed_role_ids, $account_role_ids) as $role_id) {
        $account->addRole($role_id);
      }

So with these values,

• line 220 returns an empty array, so no roles are removed from the existing user
• line 222 returns ['member'], which will be added to the existing user

so that's good, or no?

$event->markAccountChanged() is a signal to the caller that the account should be saved (because otherwise every event subscriber would re-save the account). Perhaps something is going wrong or some exception gets thrown somewhere after this, so the account isn't actually saved?

Not much I can do from here.

1)
Check the config value for 'samlauth_user_roles.mapping'. (Using e.g. drush cget or config inspector?)

- 'saml_attribute' sub-value should match exactly the name of the attribute that you see in your IdP response, containing "IT Staff". (See "SAML" main config screen to debug/log Idp response, if necessary.)

- 'only_first_login' should likely not be on, in your case

- 'value_map' should contain array ( 'attribute_value' => 'IT STaff' , 'role_machine_name' => [ Your machine name, i.e. likely not "Member"] )

2)
If that all is correct, then debug UserRolesEventSubscriber::onUserSync(). I have no idea what's going wrong there, then. The code is... fairly readable.

I've created an MR and will set it to Needs Review for the message / as an FYI, but actually, I will already push this to the main branch.

Because I need it, and because I've already pushed so much form-message changes anyway.

I will still handle requested changes to the message.

On a fresh Core install (e.g. lupus-decoupled), Core auto-installs entity view displays for Default, Teaser and RSS, so the warning shows up. It's a lot of screen space, but... the warning IMHO is really necessary (and it refers to the info message so that it itself can be shorter).

I un-confused myself and rewrote the summary to reflect the actual issue.

I want to fix this now; it's going to save me time and confusion while working on actual projects.

I did a hit-and-run review comment because I was curious after seeing the review request in Drupal Slack.

I don't know the full history, but could you have a look? (Not changing status myself.)

I just woke up to the thought of "what ) wrote in this isssue is fundamentally wrong".

It's possible that I was having a mental meltdown (somehow caused by trying to upgrade an internal project and misinterpreting details about why we precreated CE displays inside it, using a beta version several months ago).

I believe there may be something here about "printing a message when a CE Display doesn't actually exist yet", but it is likely minor.

Basically I need more information from my future self.

roderik → created an issue.

For completeness:

I made a judgment error / caused unnecessary diversion just-pre-3.0... because it turns out that I could not reuse EntityCeDisplay:: getConfigDependencyEntityViewDisplays() anyway. (The logic turned out to be more complicated and confusing in the form.)

This means I did not need the short-lived public-in-EntityCeDisplayInterface method, so I removed it from the interface and made it protected in #3475342-4: Improve CE UI form options visibility and messages → . (Reasoning: people in theoretical inheriting classes could still call it.)

This is however technically not good: there could be theoretical 'outside' code trying to call the now-protected method. So I propose to do it technically-correctly, and deprecate the public method before we do a new release, in 📌 Deprecate two EntityCeDisplay methods Active .

Fixed by direct push to 3.x branch; see commit.

@abhishek_gupta1 please remove the notifications you have (apparently) set up for the custom_elements module; I will from now on feel free to silently ignore any contributions I get through Virasat Solutions, and not credit you, if things cost us extra time. Reason:

• Review of your patch:
  • It does absolutely nothing. There is no functional change; you have just replaced a new PHP construct by an old one
  • It introduces unnecessary space changes in an otherwise unchanged line
  • Do not add comments to say exactly what the next (simple) line does. It's not helpful / adds no informatiib. Comments may be useful to say why code is doing something; otherwise, generally, just don't add them.
• In 📌 Drupal 11 compatibility fixes for custom_elements 3.x Active ,
• sarwan_varma just created extra work for us (by making things incompatible with Drupal 10.2). If we did things ourselves and ignored the contribution, we would be faster.
• I credited him anyway. But this has crossed a line: I am not going to do this anymore, or feel obliged to give any reply that costs me time for no clear benefit.

@fago for your opinion about whether 'official' deprecations like this are unneeded, this early. (I'd rather just start doing it the 'official' way.)

roderik → created an issue.