🇳🇱Netherlands @roderik

This seems like an oversight / wrong documentation on my part.

A relative path as RelayState parameter (as a request parameter attached to the POST request that also contains the SAML response) should work fine. **

So I think your IdP strips it (i.e. it does not propagate the RelayState to that POST request) if it's a relative path. Maybe most/all IdPs do that. (I should doublecheck SAML docs.)

I guess part of the reason this has not been reported before, is that the destination parameter (if one visits /saml/login?desination=PATH) is converted to an absolute URL, and not many people pass an argument to the $return_to parameter.

I should at least amend the function parameter docs/comments for $return_to.

---

** Unnecessary detail -- I was checking / wrote the below down, before my conclusion above:

• Is checked for 'being safe' in https://git.drupalcode.org/project/samlauth/-/blob/8.x-3.x/src/Controlle...
• Then is checked for access in https://git.drupalcode.org/project/samlauth/-/blob/8.x-3.x/src/Controlle...
• If the path is ignored in either case, there should be a watchdog warning.

I don't see any other code influencing the RelayState parameter.

I'm going to turn this into a reminder for me to, long-term, add something to the README and/or add a config option to skip user login (that might not be exposed in the UI).

It would be good to know if the current saveSamlSession() code is adequate for implementing such a use case.

We don't, however, map these users to individual Drupal accounts as there's no need (or desire) for the users to be registered locally

Oof! That makes things harder. I haven't heard yet, of a use case like this.

To summarize my previous response: samlauth intrinsically links "Idp authentication" to "Drupal user login". That's easier for the common use case.

But in your case,

• you need to peel those things apart again
• you likely need to store some (extra) status/data in a 'user session' that is separate from a logged-in Drupal user (which simpleSAMLphp did, and which samlauth doesn't do), in order to do this access check on those paths.

I think it's possible, though, to use just the "SAML authentication" part of this module and to remove the "Drupal user login" part, without making things too complicated. (I'm going to assume, for simplicity, that you never want to have a Drupal user login after someone returns from the IdP.)

• You'll need to overwrite the samlauth.saml service with a custom service that extends SamlService
• Your extending class can 'empty out' doLogin() (or change acs(), which is the only caller of doLogin(). But I assume leaving acs() is easier).
• This is assuming that you don't want anything registered in the authmap table. (Also, I am not going into the horribble piece of spaghetti that is the doLogin() execution path... because you won't need it / will remove it.)
• Then I advise experimenting with saveSamlSession():
  • There is a concept of a "Saml session", separate from the Drupal login. But ss you can see from the horribly large comments, it's an afterthought / code that has just stuck around since v1.0 and not (to my knowledge) been used well.
  • Decide what data you need, in an access check that you do on the other page loads.
  • Then store that data somewhere you can get to it. (getAttributes() is available here if you need it)
  • As you can see, currently the 'session data' is stored in PrivateTempStore (and you can just add your data next to it). I'm personally not convinced that this is better than just storing this data in $_SESSION -- because as soon as you store something in PrivateTempStore, you automatically/necessarily get a session anyway. I guess it depends on the nature of your data.

I.... think that works without further complications?

If you discover things that need to be patched along the way, I'm open to it. Although, until other people come along who want the same thing, I can imagine this is not going to turn into a configurable option to "skip user creation/login", but instead just a note in the README that this can be achieved with the above.

isAuthenticated() does not exist, because it addresses an issue that is specific to SimpleSAMLphp / does not exist with SamlAuth.

See the two images at https://www.drupal.org/docs/contributed-modules/saml-authentication/usin... → if necessary:

• with SimpleSAMLphp, SAML auth happens outside of Drupal. Then the user is redirected to Drupal, with a cookie set -- and Drupal still needs to figure out whether the at-that-moment-anonymous user is allowed to be logged in. (using isAuthenticated(), which -from my memory- checks the cookie.)
• with SamlAuth, SAML auth happens inside Drupal, during the POST request to /saml/acs. And the SamlAuth module immediately logs the user in, during the same POST request. Either the user is logged in, or the user is presented with a login error. There is no other 'vague in-between state' like with SimpleSAMLphp auth.

Is it possible that the array is empty due to the IdP configuration? Or, am I perhaps misunderstanding at which point the array is actually populated given the current state of the SAML authentication flow?

getAttributes() only returns data while processing the /saml/acs POST request, not on any other page load -- and only after SAML authentication has already happened and was successful.

Without knowing the specifics of what you are solving, I am guessing:

• you're trying to do something during some event very early in the page load -- which is not a concept that works anymore.
  • you should be doing the same thing immediately after successful SAML authentication. There's an event for that: SamlAuthEvents::USER_SYNC. See the comments there. See UserSyncEventSubscriber for an example implementation.

  Note on this event, SAML authentication has already happened and your user ($event->getAccount()) is going to be logged in (except if you or some other event subscriber throws an exception or something horrible happens while saving the user account). So you don't need to do an isAuthenticated() check.

  Or, alternatively, if you want to do something after authentication and Drupal user login has happened, and you cannot cancel it in any way anymore: check the ExternalAuthEvents::LOGIN event. SamlService->getAttributes() should work there always, if you need the attribute values (though the service isn't injected so it's slightly ugly).

  This means you need to do some rewrite work, for this different concept. But the result will probably be (conceptually) simpler, I'm guessing.

Mustapha did the tracing for this, and made a first change. I picked it up to check how exactly the change should be implemented (and to confuse myself on the way).

I've tried to improve the code so the block provides a proper CE response. When done so the block rendering should pick up the CE wrapped in the render-array. But it seems the views/block rendering pipeline wraps the render array somewhere, so it ends up markup instead of json still.

Yes, the block rendering does pick up the CE wrapped in some render array:

The call tree is:
1. LupusDecoupledBlockRenderer::getBlocks()
2. Drupal\views\Plugin\Block\ViewsBlock::build()
3. Drupal\views\Element\View::prerenderViewElement()
4. Drupal\views\ViewExecutable::executeDisplay()

4. returns the simple '#theme = custom_element' wrapper, and 2 + 3 add all the other stuff around it, to 'complete' the render array for a view.

(...) the block rendering should pick up the CE wrapped in the render-array.

I assume this means that you want to do a combination of two things:

A. pick the CE out of above ViewsBlock::build() structure;

B. set the CE instead of the rendered HTML, into $output[$region], in getBlocks(). (Until now, this is never done.)

About "B": OK, no further remarks.

re. "A": this feels to me like, at least in theory, 1) we could be throwing away part of the rendered output (produced by e.g. theme functions set by the block plugin; 2) we could be breaking compatibility by changing HTML output to something else. So I felt like I should put some thought into when exactly we are 'allowed' to do this:

• If the CE is the only render-element-child, like this specific example? Sure. We can throw away the theme-wrapper function.
• If there are several render-element-children, all CEs? Maybe, but...
• ...what if there are several render-element-children, some of which are not CEs? We can probably renderRoot() the non-CE render-element-children individually, and add them as drupal-markup CEs (like is mentioned in 📌 Improve CustomElement::createFromRenderArray() to better handle entites that render to custom elements Active )?
• ...and what if they're not all first-level children? Do we need to recurse, then?

I've also seen 📌 Improve CustomElement::createFromRenderArray() to better handle entites that render to custom elements Active , which proposes to do something like this inside CustomElement::createFromRenderArray(). But thinking about the above points just confused me.

So, in the end, I added only the first bullet point, directly into getBlocks(). That's what we need right now. And I can't justify the testing time for making this more generic / decide how 'backward compatible' the other cases should be... unless I'm explicitly told that we should address those cases now.

@fago see MR. Is this the code change you want? Or am I misinterpreting some things?

Might be fixed by https://git.drupalcode.org/project/config_override_warn/-/merge_requests... ( 📌 Automated Drupal 11 compatibility fixes for config_override_warn Needs review ), specifically the extra installEntitySchema() call. So apparently that fix is not only for D11?

Note - I didn't check, just leaving a quick note.

Thank you for the detailed report.

First - the less important thing, but important for myself:

Per "2)" in my response #9: I still have no clue why you're getting this ugly error and there is not an earlier 'real' error message about the name being empty. But you confirmed your configuration, so I have something to manually reproduce and test, some time.

This issue is "Needs Work" for at least that point: reproducing and testing, and then seeing whether/where I should add a real error ("Your user name is not being populated from SAML data, your 'User name attribute' setting is likely misconfigured" or something like that).

---

Now, to the details of your setup:

You only sent the <saml2:NameID Format="urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified">correct@emailaddress.com</saml2:NameID> part of your response.

I am going to assume that your SAMLResponse contains no attributes at all. Attributes look something like (real example at https://www.samltool.com/generic_sso_res.php) :

      <saml:Attribute Name="uid">
        <saml:AttributeValue xsi:type="xs:string">test</saml:AttributeValue>
      </saml:Attribute>

Therefore I am also going to assume that you want to have Drupal users created whose name == the e-mail address == the NameID that you are getting back in the response.

If my assumption is correct, then this is a combination of
1. Your configuration is wrong
2. The module is not giving you any hints about how your configuration could be "fixed". So the UI is wrong/incomplete.

Per 1: If you indeed have no attributes in your response, then the configured "NameID" attribute (for user name) does not exist, and the configured "urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified" attribute (for email) does not exist... because no atrributes exist at all.

Note again that "The NameID" and "SAML Attributes" are completely separate things. And (I assume from your comment) you only get the NameID, and zero SAML attributes, in your response.

Per 2: this module only recently implemented support for "NameID" (as opposed to an atrribute) for its "Unique ID" setting.

I failed to, at the same time, adjust the UI for the "User name attribute" and "Email attribute" fields. (And you're the first to semi-explicitly tell me that your responses have zero attributes.)

Solution: this 'trick' should work: set !nameid for the value of both "User name attribute" and "Email attribute" settings. Note: must be lowercase, starts with !.

This issue is also "Needs work" for fixing the UI to make that clear.

Maybe there's a prior question of whether this module is still needed?

Just for the people only reading this issue:

Yes, it's been established that this is still needed, in #3462541-4: Difference with what is now in core? → and further.

So a merge + release is appreciated.

This module can be useful until the mentioned render array attribute gets widely used in the contrib space.

Yes.
In addition: this module also shows overrides for config entities. (It's perfectly possible to override config entities' properties using $config in settings.php.)

We have somewhat simple entities, in a form that extends EntityForm, and this module correctly reports (I changed the names):

In the config file MODULE-NAME.ENTITY-TYPE.ENTITY-ID
- The value for PROPERTY-NAME has been overridden.

I don't see a plan to support this in Core. (And I don't know enough about overriding config entities, to know if changing the code to support this, would be simple. But at this moment, all the code is in ConfigFormBase, which != EntityForm.)

I came here from 🐛 Expected argument of type "array", "string" given Active and this MR inspired me to make a MR there. (The code is different: this one catches a crash in checkValues(); #3444730 catches a crash in validateValues().)

While I have not reproduced this... I think it you can probably argue that

• this does not need tests at all, because we can/should just assume that
  • checkConfigSchema() can throw any old exception
  • and checkValues() just needs to take care of those and never choke
• ...so all you need to do is check the phpcs error and then set to review without tests?

I admit that this in my validateValues() case, that is clearer to me than for this MR. (Because this calls checkConfigSchema(), which I don't know, whereas my case calls Typed Data stuff, which is "urgh, scary black box, we must be prepared to handle any exception that could throw".)

I was inspired by the recent 🐛 Crashes on leftovover themeconfig Active PR to look at the code again and could make some sense of it (because I now understand/think that I can ignore the impenetrable TypedData code). #3359846 does (almost) the same thing to different code, btw.

Proposed fix included in new MR. And...

• ...do we actually need tests (and steps to reproduce) here?
• Or can we just assume that
  • TypedData stuff can throw any old exception
  • and we just need to deal with it (i.e. catch it and turn it into a violation)?

I think it's the second. I think this just needs a try-catch + return error, and I don't need to bother about writing tests that cause any exception.

Manual testing done

• UI:
  • Before: fatal error
  • After: the row for the offending config item reports "1 error"
  • The actual error is reported in the list/detail screen. This does not change / did not suffer from the fatal error.
• drush config:inspect:
  • the fatal error is never reached, because inspector->validateValues() is called before inspector->checkValues(), which is (I think) therefore never called and doesn't get a chance to choke.
  • however, if it was called, this try/catch gives better output. (I tried. Before the fix, you only get the fatal error reported by drush.)

Reproducing the error (though I'm arguing it's unnecessary)

1) we have #19

2) Putting comments #12 and #20 in context:

If you have a firmatted text field, and you change the default value to

default_value:
  format: restricted_html

and import that: this will cause the fatal error in the issue title.

Random useless info

This is of course a completely bogus value: the actual default value would be:

default_value:
  -
    value: '<p>this is my default text</p>'
    format: restricted_html

...and Core does not allow an empty "value".

Our config did, however, have only a default format in the config, like:

default_value:
  -
    format: restricted_html

...which, I assume, was somehow necessary to work around a bug / select the correct default format, when allowed formats was still implemented in contrib (allowed_formats module). But:

• Core does not allow to save a default format without a value, through the UI
• a drush config:export will re-export this format-without-value in the above format which causes the fatal error when you re-import the exported file.

I rely on other people to specify their 'config sync' policy / best practices. So I'll rebrand this a task.

People must be doing this. But so far, I don't remember anyone really commenting on it.

Are they using config_ignore? If so, what are they ignoring? Just the entity ID (provided the certs are in files)?

Do they want to use multiple IdP configs for this? ✨ Multiple IDP Support Needs work

When there's a somewhat authoritative reply from someone, we can determine if this needs documentation, or other work.

Thanks for the re-explanation, I'm not always fast picking everything up :)

Regardless whether this solution would be suitable for your needs: the fact that a UserVisibleException thrown in a USER_SYNC event subscriber is a bug that should be fixed. (And noone's reported it so far + I somehow failed to spot it + there are no tests for this.)

I "plan to soon" test and extend SamlController::handleExceptionInRenderContext() to also recognize the wrapped EntityStorageException, then. (and/or any exception that wraps a UserVisibleException...)

That's arguably a hack to counter a hacky situation (the fact that the event is dispatched during hook_user_presave). But it is a recognizable specific hack without further issues. And for the moment I'm still sticking to the hacky situation and living in hope I'll tweak externalauth someday.

(Because - better summary: otherwise I'd need to give up on either dispatching the same event for new and re-logging-in users, or having protection against saving half-constructed user entities with bogus data.)

(Aaaaaaaactually re. "done during saving":

There are complications. The gist is: I would like to do separate the user sync out from the eventual user_save(), but that requires a rewrite of the externalauth module if you never want to run the risk of ending up with a saved user with half-incorrect data.

I think. It's been a while since I looked at the details.)

> I somehow assumed that the UserSync event would only run for new users

OK, that's cleared up then.

> It does specifically for new users run quite late though, only when already saving the new user. That will also catch and rethrow the exception in the storage handler. This would also break the ability to display a specific message to the user.

Well yeah, interpreting the SAML message and saving the new user is basically the only thing it does. I never considered that "late". (I guess I never cared about the catch + rethrow you mention? What mattered to me only is that it gets caught again in the controller. Also: apologies for the code spaghetti.)

For displaying a message to the user when login (and/or creation / sync) fails, you can throw a Drupal\samlauth\UserVisibleException. Does that help?

I guess I could throw an exception during the user sync event, but if the attributes change for an existing user, it wouldn't prevent the login.

Not sure I'm interpreting this correctly.

• The login is always prevented if you throw an exception.
• What you can't do (well I guess you can, but not in a recommended way), is prevent the login but still update the Drupal user data with data from the IdP. I wouldn't expect anyone to want to do that... so... just asking for confirmation: is that really what you want to do? I never considered there would be a use case for that.

roderik → created an issue.

Rebased the old MR because the underlying code changed a lot with 🐛 When editing a custom elements page view php errors are produced by renderer Needs work (code was moved from the controller to CustomElementsPage plugin). Tested that output is the same as before.

The MR is not ready, because of now 2 reasons:

1) CustomElementsPage and CustomElementsBlock contain pretty much the same code though they extend different classes. This wants a trait.

2) The originally reported issue still exists. I need more time to properly comment on this, than I have right now.

roderik → changed the visibility of the branch 3363234-subrequests-request-uri-patch-8678 to hidden.

@benellefimostfa

* Tests should be green after you merge/rebase.

* Check phpcs pipeline errors. (It's a bit irritating that static analysis for 3.x is "orange" at the moment; I will fix phpstan to be green soon, so it is better to spot.)

Reviewed.

I was wondering if we should test the HTML normalization standalone. In that case, we should likely add that as a "quasi output format" to some CustomElementBaseRenderTest tests.

I'm not sure whether I care much about this at the moment, I need to take a look again later (after changes are implemented), whether we are exercising all normalize code (e.g. normalizing of some array values); if we aren't, then it likely makes sense to do it.

@pfrilling sorry, I should have added the change in caching the form, to MR!139.

If you merge + push to 8.x-1.x without this change, I guess that's fine and we can wait until upgrading to the 3.x branch, until we see this fixed in practice.

(This change is not closely related to the reported issue, but it's valid, I can confirm that I've seen the caching issue.)

That's MR!139.

(I'm going out on a limb that you're not needing to add tests there.)

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

CE release is out; requirement added to composer.json. Build is green.

Note this is a new "require" line because previously, we depended on custom_elements only indirectly through lupus_ce_renderer..

I was tempted for a moment to drop support for CE v2, just because we might introduce some functionality later that depends on CE v3 without realizing it.

But 1) that's not up to this specific issue; 2) I can't really think of such functionality (yet; let's see about CE v3.1)

As an aside: the recipes that ship with configuration all require custom_elements v3. But I assume 1) they would error out when trying to install config onCE v2; 2) no new installation will ever install v2 anyway.

roderik → changed the visibility of the branch 3504718- to active.

roderik → changed the visibility of the branch 3504718- to hidden.

Fix in 🐛 Error enabling layout_builder Active . If you approve that issue + want, I can release custom_elements 3.0.2 tomorrow, and add a MR here that depends on that minimum version.

This needs a review, because the code is 'ugly': I don't see a way to know that "layout_builder is being installed" without hardcoded assumptions. (And that's why the comments are big.)

I think we have no other options / it's fine.

• It errs on the "safe" side, being: only skip when it encounters the exact Layout Builder classname (while Layout Builder is not actually installed).
• This does mean: if the class is somehow renamed, the fatal error returns.

I checked the entity_type_alter behavior manually with xdebug enabled, and it does the expected thing:

• only skips while layout_builder is being installed
• works normally,
  • at other moments during the 'drush en layout_builder' command
  • in normal operations
  • during install/uninstall of other modules, either with or without layout_builder enabled.

I only tested with drush. Since the only 'interaction' is with the ModuleInstaller service, drush vs UI install can't make a difference.

roderik → created an issue.

Minimum modules required to trigger the error on my ddev instance:

Note, no lupus_decoupled modules are involved:

$ drush si
$ drush en -y custom_elements
 [success] Successfully installed: custom_elements
$ drush en -y layout_builder
The following module(s) will be installed: layout_builder, layout_discovery


In EntityTypeRepository.php line 111:

  The Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay class does not correspond to an enti
  ty type.

So this seems a custom_elements bug, and feels related to the fact that we are 'unlinking' the LayoutBuilderEntityViewDisplay class:

function custom_elements_entity_type_alter(array &$entity_types) {
  // Use the right class depending on layout builder being used.
  $class = \Drupal::moduleHandler()->moduleExists('layout_builder') ? CustomElementsLayoutBuilderEntityViewDisplay::class : CustomElementsEntityViewDisplay::class;
  /** @var \Drupal\Core\Entity\EntityTypeInterface[] $entity_types */
  $entity_types['entity_view_display']
    ->setClass($class);
}

I will trace this and add a custom_elements issue today.

The error looks bad but it seems not to do anything. I find nothing in the status report, no strange behavior.

Uploading patch for 8.x-1.x to this issue too, for 'fixed' reference for our build system. It can apply to 3.x too, the code is the same, it's just 18 lines higher up in the .module file.

I admit to only live-testing the 8.x-1.x version on our system, but I don't really see how there would be a difference between the two versions in this respect.

Pipeline for 3.x MR is green, so I assume pipeline for 8.x-1.x MR is not failing harder than usual.

roderik → created an issue.

@bourgeoy I still have no idea what is exactly happening so I need your / anyone's help to debug and provide technical details, so I can judge what is going on (and then perhaps provide better error messages).

The communication was haphazard (and sometimes wrong from my side), so I'll try to re-summarize myzelf:

I guess first we need to know if at this point before externalauth is called, your $account_data['name'] is empty.

If it is empty, then there are two separate things going on:

1) There is probably something wrong with your configuration or the data that your IdP sends. The "Contents of the user name attribute" in the message from the IdP should always be populated.

2) Regardless of "1" likely being your actual problem: I am still expecting the username to be filled with something (specifically "samlauth_THE-UNIQUE-ID"). Details in my comment #4. This is why there is no error message from this module: I can't implement one, until I see where and how the username would be set to NULL.

If it is not empty:

Something outside of the module is surely overwriting the username with NULL, just before saving it. I don't know what or where, so I also don't know where to give a better error (if possible).

Embarrassingly, 3.x-1.10 itself contains an error that triggered the warning and therefore should be fixed. Your issue is probably fixed with 🐛 Cacheability Metadata Leakage Error on SAML Login with Samlauth and r4032 Redirect Module Active . 8.x-3.11 is out now.

I should have seen it, of course. Unfortunately I don't use this module actively anymore, so it takes time until I spot things. And I'm happy that people like you are actively tackllng some issues.

Finally a new 8.x-3.11 release is out with fixes now, and the list with outstanding bugs and clearly-needed additions is cleaner than ever. (Dare I say zero?)

The log that potentially bothers people under some circumstances ( 🐛 Code leaked cacheability metadata Active ) is still present in the module, and me reading up on whether it is needed, is still outstanding.

Thanks for adding tests! (I haven't looked at the exact reason why they fail for next-PHP-version; I think that happened in the main branch too, and it went away somehow. I'm also hoping to clean up the bogus warnings sometime soon.)

I was doubting whether to cram this through right now, because I'm going to do a new release and this is the only outstanding thing. Alas, I feel uneasy about one thing that I don't want to hack now, so it'll have to wait; I'm giving that back as/for feedback.

I don't have a setup with different hostnames, to test this immediately. So I think this will leave you with general support from others in this queue, for now.

Assuming you mean the seckit module (which I am not intimately familiar with):

* Acquia supports seckit, and I also know that several of their hosted sites use the samlauth module. So it's remarkable that I haven't seen this issue before.

* It would certainly deserve documentation, if we find that something needs to be preconfigured.

* https://www.drupal.org/project/seckit → mentions https://www.drupal.org/project/seckit_override → -- is that a way to tell Drupal not to complain about saml/login? (I mean... assuming that you can see/verify this request really is right. But I assume you can see that.)

Thanks for the reminder. I guess I should have not closed this stale issue myself, because of the missing credit that I can't assign.

In principle it seems better to assign here -- 📌 Add logo Needs work was just a throwaway reminder issue (without credit) to add the SVG file. But since I don't really know the maintainer situation here and I don't want to cause more indirect communication... I added the credit in there.

Duplicate with 🐛 Error encountered when accessing SAML authentication mapping configuration page Active (which was open earlier, though it didn't contain any useful code yet when this was opened). See there.

This has grown too complicated to administer properly to everyone's satisfaction:

• A new issue was opened that, I think, also fixes the issue. That is: while this issue was already/still open, but before the second MR was created.
• A new patch was added in January, but it doesn't say why / why the MR wouldn't be good.
• The MR in this issue was more or less copied to externalauth 🐛 Updating to v2.0.4 causes authmap view to produce an unexpected error Active (which I reviewed and which was committed to v2.0.7) but was sloppy; it contained references to samlauth instead of externalauth. I still doubt that it was actually needed, because 'uid' should have worked fine for the externalauth case.

In the end,

• I re-investigated my old code, and improved it so that it doesn't rely on hardcoded alias names at all - in externalauth 📌 Improve views integration Active
• I stripped code from this module as far as possible (as long as #3503865 is not committed to a released version yet) and committed that. (I did include the change that is also in patch #17 == the 🐛 Authmap view is breaking - admin/config/people/saml/authmap Active MR. While investigating, I saw that users_field_data_authmap_uid is neither necessary, nor theoretically correct. If that causes issues for anyone: comment.)

To complete the story (though it's not important):

Per my last message, I have no clue how people are still seeing errors.

I again suspect that the externalauth module was fine already. Because only after I made all the above comments / review, did I see that the patch uploaded here was wrong, and included a samlauth route instead of an externalauth route.

So at the very least, it wasn't tested properly.

I made a quick last-minute fix commit, but did not have time to dive in further, anymore. (And the merged MR does no harm, so... whatever.)

Now that I've had time, I sat down and had a good look at my old code, and opened 📌 Improve views integration Active which removes all the hardcoded aliases and hopefully will never cause issues like this again. I'm going to finally fix up the samlauth equivalent of this issue, make it dependent on externalauth v2.0.7, and hopefully never look back.

(random note: I apparently cannot change my entry in the contributions list back to "roderik, as a volunteer", which it should have been in the first place. Or at least I don't know how. Oh well, shrug, I don't care that much.)

roderik → created an issue.

That's really not enough info to qualify as a bug report for this module.

If you get no good messages and no logs in watchdog, just "Access denied"... then I bet it's not the samlauth module but something else (domain module?) hijacking this. And we need more debugging info to judge what can be done about it.

Reviewed:

Saw it in action, but also: this patch looks good from just code inspection, without side effects:

• There doesn't seem to be a reason that $form_info['label'] needs to be converted from [empty value] to "". (It turned "0" into "" before, but I think we don't care.)

Created a MR for the gitlab. I hope I can still change from implicit-Needs-Review to RTBC. (The only doublecheck to be done is, did I change anything from patch to MR contents.)

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

Changed scope of this ticket, to include things I want to do (to have a useful set of tests without undocumented legacy cruft).

I didn't know what this meant in detail, when I created this ticket. I just copied it from #3455496-5: Drop simple and single+simple slot normaliziation styles → .

Now I do., and this is covered by 📌 Create test for checking markup serialization Active .

At least: let's put it like this: The current setup of the CustomElement class/setSlot() method

• means/implies that in the vast majority of cases, setSlot() sets single-slot normalization style
• implies that this only influences JSON output, whgucg in some circumstances (i.e. code that does 'unexpected' things) can produce inconsistencies between JSON and Markup output.

If we determine that the last bullet point needs changing: 🐛 setSlot() + addSlot() + output inconsistency Active is open for that.

Reviewed. This is good. But done:

• In https://git.drupalcode.org/project/custom_elements/-/merge_requests/109#..., when I said "single setSlot() call" and "above", I meant the test above my comment that contained a single setSlot() call. Because that would test/document an isolated addSlot() call. You have extended a more complicated test, so... I now added this myself. I respected/took the code setup that you used in other tests.
• testSlotOperationsWithMultipleChildren() did not contain the Vue3 version yet, so I added it for consistency - and changed the test comments a bit.
• I renamed the class, and commented that it actually tests the combination of CustomElement + CustomElementsNormalizer.

I don't usually merge issues that are still set to Needs Work, but review was requested elsewhere and we can continue in ✨ Create a HTML-to-CE parser Active if needed.

roderik → created an issue.

roderik → created an issue.

This looks good, and I also learned things about CE in the process. (The outdated issue summary is a sign of how little I knew about CustomElement basics back in September -- I likely won't bother updating it.)

I have some requests for additions though; see MR comments.

Please change ticket status to "Needs review" when I should review.

Also: I will want to have a larger comment on the class (to say that it is not only testing CustomElements but also rendering) and possibly rename it. But I think I'll just do that myself afterwards, and not bother you with that.

adding markup in this case is unusual, so not something we have to cover imo.

I actually think we'll need to cover this because it will become more important. But we can add that coverage in ✨ Create a HTML-to-CE parser Active after merging this.

@fago needs review. If the "Alternative A" output (in the issue summary) is OK for a default, then I will amend this MR to also support multi-value file/image fields.

My first thought would be to add multi-value support to the "Flattened" formatter, so that its output becomes non-flattened for multi-value fields. But I know you've spoken out against that, so I'll instead copy some code into FileCeFieldFormatter to support the two cases, and possibly make it not extend FlattenedCeFieldFormatter anymore.

This still means that FileCeFieldFormatter will output a somewhat different structure for single- vs multi-cardinality fields. (Just like DefaultFieldItemListProcessor does, basically.)

Only adding for completeness: if we remove FileReferenceFieldItemListProcessor altogether (which is responsible for producing the Drupal-HTML output), and let it fall through to the DefaultFieldItemListProcessor, the output becomes:

    "featuredImage": {
      "element": "field-image",
      "mediaImageTargetId": "3",
      "mediaImageAlt": "The Drupal logo",
      "mediaImageTitle": "",
      "mediaImageWidth": "1080",
      "mediaImageHeight": "1080",
      "content": "3"
    },

...which is not good: there is no "URL" property (and instead there's an opaque entity ID).

I'm guessing FileReferenceFieldItemListProcessor may not have been used in years (and therefore we haven't really paid attention to this old code yet), because our custom internal distro uses custom processors.

roderik → created an issue.

Confirmed: apparently it is specifically the "PHP deprecation" that caused Gitlab to report failure.

Fixing it makes phpunit report "orange" in Gitlab (for all the other deprecation messages reported by code in e.g. paragraphs module), and the full job report "green".

The fix was exactly the same as in the referenced webform issue: import config entities through entity API, not "plain" config API. (Apparently then they get UUIDs and there is no "NULL" condition in a query.) I just needed to modernize some code before applying the fix on top of it.

roderik → changed the visibility of the branch 3500903-phpunit-failure to active.

roderik → changed the visibility of the branch 3500903-phpunit-failure to hidden.

The phpunit failure is not for this change; it's only depecation messages which I'll fix soon in 🐛 PHPUnit failure Active .
So

• tests actually 'succeeded' (for markup)
• and for (the changed) JSON output we have no tests yet. I should add these after the code change is approved (either in this issue or in a followup).

About alternative implementation

I just want to mention this for completeness - Disclaimer: there are gaps in my knowledge

It feels to me like, in principle, given the fact that the above 'JSON array' is apparently invalid... the Normalizer is responsible for not producing invalid output.

So, instead of the change in the MR ("bulid/render-into-element phase"). the "render-into-output" phase should have the fix.

That would look something like changing some code in normalizeSlots() to this pseudo code:

if ($element->hasSlotNormalizationStyle($slot_key, CustomElement::NORMALIZE_AS_SINGLE_VALUE)
    || (ThisSlotOnlyHasASingleValue($slot_key) && MyFormatCannotHandleArrayValues()) ) {
  $slot_data = reset($slot_data);
}

Where MyFormatCannotHandleArrayValues() is either always true, or can be made to check the $format parameter for the normalize(mixed $object, ?string $format = NULL, ...) call.

However,

• it is harder for me to see if that has wider implications
• if we would implement this, I'm starting to wonder if NORMALIZE_AS_SINGLE_VALUE still has an actual use in the future, or whether we should also start to deprecate that
• I have no clue if all consumers (JS frameworks) would have problems with array values. (Hence checking the $format parameter.)
• We are not using the $format parameter for anything yet (even though we soon will, in an upcoming change to ✨ Create a HTML-to-CE parser Active that I or Mustapha should still document).

So, this is too 'unknown' for me to make a MR out of, unless you actually agree that this should be the direction. (The current MR that changes DefaultFieldItemListProcessor, feels more in line with... how the code is currently set up...?)

roderik → created an issue.

For completeness: "Needs review" status is for my last comment, not for the code.

I am considering just replacing the code by a variant of my "Another alternative" (last paragraph+bullets)... but this hasn't risen to the top of my to-do list yet.

Testing phpunit locally:

I get lots of "deprecation" messages, which do not show up in the GitLab pipeline.

I do get one "PHP deprecation" message too, which does show up.

• In CustomElementsRenderMarkupTest and ...Vue3Test

• 3 tests triggered 1 PHP deprecation:
  1) /builds/project/custom_elements/web/core/lib/Drupal/Core/Config/Entity/Query/Condition.php:39
  mb_strtolower(): Passing null to parameter #1 ($string) of type string is deprecated
  Triggered by:
  * Drupal\Tests\custom_elements\Functional\CustomElementsRenderMarkupTest::testNodeRendering (3 times)

I've ignored this until now, because I thought it was a Core thing.

Some googling got me to #3302838-18: Querying with NULL values results in warning mb_strtolower(): Passing null to parameter is deprecated → and then also #30 which references 🐛 'filter.format.' . WebformHtmlEditor::DEFAULT_FILTER_FORMAT missing UUID Fixed == a contrib fix commit.

We should probably do something similar, and the error (absence of a UUID for a config entity?) is probably somewhere in importPartialThunderConfig().

roderik → created an issue.

The build failure (phpunit) is certainly unrelated, I'll check it somewhere else.

(It seems to me that phpunit is only reporting deprecations, in Core code, so it feels like it should exit with warning instead of error....?)

roderik → created an issue.

@dydave: can you get to a reproducible situation? (Does your custom module have imported config and/or a schema in config/schema that you can isolate / share?)

The first thing we'll need to do here, is isolate a situation that causes the crash. And ideally make a failing test case out of it.

I'll re-try based my earlier message... some time. Because I'm still not sure what exactly fails. (I need to re-test what goes wrong with our "restricted_html" config because it has changed since I tested/wrote #12.)

Oh, right. I should have seen that this login method was a JSON API endpoint !== the form submission.

Point 1: this works. (So, in the end, it's just small changes to get/derive the view mode from the right place. Plus tests/etc.)

Point 2: already done, on https://lupus-decoupled.org/advanced-topics/searches

so, RTBC.

Release was created.

Used gitlab code editor for the first time.

roderik → created an issue.

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.