🇳🇱Netherlands @roderik

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.

@fago you had reported this weeks ago, but I was unable to reproduce it at that moment. (Likely because I only checked sites where a default CE display was already saved.)

The fix is straightforward. It just took me a while to work out

• that this is IMHO the appropriate location to add it;
• why the tests did not notice things and how they should be fixed. (Answer: for all auto-created / auto-processing CE displays, I should check which entity view displays the fields come from. Now done, and I trust the tests. The combination of permutations is... daunting, but I kept things "as readable as I can". Just don't read them, though.)

roderik → created an issue.

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

@fago I want to do this, but.... since we have been "ping-ponging " several times already about the code change in EntityCeDisplay, I didn't want to commit this without you seeing that particular change.

With this MR, the "hidden: null" value is really truly gone, so it cannot cause errors anymore. (It can only cause errors as long as you update this code without having run the update hook yet.)

-----

Just some random details for documentation:

Behavior that has confused me for a long time already / what I learned:

• After the update hook runs, active configuration holds no "region" or "hidden" values anymore,
• BUT... a "drush config-export" will still write those values into new files in an empty config/sync directory! (Until you change a 'real' value in the config entity.) Reason:
  • It uses some temporary file storage internally, which is not updated until you actually change a value
  • Before exporting, the active configuration is compared with the temporary storage
  • Since (because of the config schema) no "real" differences are found... the temporary storage (which still contains "hidden: null" etc) is used for exporting!
• There is no good accessible method to access/clear this temporary storage, so I'm leaving it alone.
• If those files (containing "region" and "hidden: null" are re-imported, these values do not end up in active storage; they are filtered out (because of the config schema). So there's no risk of bugs there.

roderik → created an issue.

Done.

MR 102 is made against 3.x.
It gets the language from the entity where possible. So the behavior is better in this regard, but now also inconsistent. (Reasons were given in #5 and #9.)

MR 101 is still open for comparision; its behavior is 'not better, though more consistent'. It is against 8.x-2.x.

Since both branches work exactly the same, I will commit a similar one to each. That is: e.g. if MR 102 is approved, I will close MR 101 and commit an equivalent of MR 102 to 8.x-2.x too.

roderik → created an issue.

Setting to Needs Work after editing my last paragraph for my plan... for tomorrow.

Extending the config schema has been done in various issues, mostly 📌 Separate configuration of CE field formatters and display components Fixed (but it's been updated since, in issues that modified the CE display properties).

This can be closed.

We'd have to forward the selected langcode. So we must pass the entity language as $langcode when calling this.

Next, the bug might exist in other processors also. When we fix it, we should fix it in all of them.

In principle I agree. This is what I was trying to do, when encountering the problem with paragraph references that I described previously.

Since you want this too, I checked further into what the problem is:

The only way I know, to get "the entity language", is doing $field->getEntity()->language().
But that returns the original untranslated language, for any non-translatable field. So, for untranslatable entityreferences (e.g. paragraphs), that's worse than now: all paragraphs in returned JSON will be untranslated.

$entity = get_some_entity()
// $entity->defaultLangcode:       'en'
// $entity->activeLangcode :       'de'
$field_item_list = $entity->get(NON_TRANSLATABLE_FIELD);
// $field_item_list->langcode:     'en' 
$check_entity = $field_item_list->getEntity();
// $check_entity->defaultLangcode: 'en'
// $check_entity->activeLangcode : 'x-default'

This is not specific to paragraph references; it's for any non-translatable field. The reason is in contentEntityBase::getTranslatedField(), which is always called by $entity->get():

• It calls itself recursively with language 'x-default'
• the recursive call sets the 'x-default' entity in the fieldItemList.

So I'm stuck on this. (I don't believe paragraphs can be the only non-translatable entityreferences...)

I was contemplating writing a helper function to check if $entity->activeLangcode == 'x-default', and then getting the language code from ConfigurableLanguageManager::getCurrentLanguage(CONTENT-LANGUAGE) instead as a fallback. But $entity->activeLangcode is not accessible. So.... mmmh.

So the only thing I know to do is always get it from 'the countext' =~ ConfigurableLanguageManager::getCurrentLanguage(CONTENT-LANGUAGE). Which is what the current MR does.

Note: this is exactly the same for 3.x as for 8.x-2.x.

I stumbled across this issue while trying to debug something (and I cannot find another related issue). I have no opinion on the original issue description here, but just want to drop a note.

This is some current behavior of a non-translatable field on a translated entity, for better or worse:

$entity = get_some_entity()
// $entity->defaultLangcode: 'en'
// $entity->activeLangcode : 'de'
$field_item_list = $entity->get(NON_TRANSLATABLE_FIELD);
$check_entity = $field_item_list->getEntity();
// $check_entity->defaultLangcode: 'en'
// $check_entity->activeLangcode : 'x-default'

getEntity() gets the untranslated entity. I do not know if this was intended, or if this is documented anywhere...

but from reading the patch, I believe that this will change to $check_entity->activeLangcode == 'de'.

Maybe that's a good thing. But I'm guessing it needs to be taken into account, and documented somewhere.

(And maybe there needs to be a test for this, going forward?...)

@fago for your review, see MR:

• Do you think we can just add this change into stable 8.x-2.x and do a new release? (I think we can and this is 'just' a bugfix, not a behavior change, per previous comment)
• Do you think calling getTranslationFromContext($entity, [ no explicit language ] ) is the correct thing to do? If you know the answer is "yes", you don't need to read my above ramblng.

(Never mind the .gitlab-ci changes. They were needed to make tests green again and are documented in 🐛 Make Gitlab D11 PHPUnit job run successfully Active )

Reviewed:

• Code is good. (I injected the service after we talked. I also added a tiny change to CustomElementsGenerator for consistency.)
• There is no other processor code that directly manipulates $data->entity or a reference->getTarget().
• There is other processor code that calls CustomElementGenerator::generate($data->entity) without a $langcode parameter.

This extra call (see MR) brings DefaultFieldItemProcessor in line with those other generate() calls, so this MR feels logical. (Note lupus_ce_renderer's CustomElementsController also always calls generate() without a $langcode parameter.)

I did not know what language does, so I needed to think and test, on a clean lupus-decoupled install. Questions that came to mind:

Is this change considered a bugfix and not a behavior change?

I think so. Some behaviour could change, but

• Per above: it just becomes more consistent.
• The current behaviour that returns a translated DE node with an untranslated EN taxonomy term, is clearly wrong.

Are we forcing the 'proper' language?

The current code (that calls EntityRepository::getTranslationFromContext($entity, NULL) -> ConfigurableLanguageManager::getCurrentLanguage(CONTENT-LANGUAGE) forces "the current content language, as negotiated by whatever negotiator is active".

Milan told me that with this MR, he was actually seeing an EN taxonomy term while outputting a DE node, when his own account language was set to EN. That sounded dangerous to me / I don't think we want that. However, I could not reproduce it... and also cannot really explain this, because the 'root' call in CustomElementsController leads to the exact same way of calling getTranslationFromContext() as the changed call in this MR.

An alternative is to force the language from the entity, for any referenced entity. That is: to explicitly pass $data->getEntity()->language()->getId() as an extra parameter to any generate() or getTranslationFromContext() that is called in any processor.

I thought that conceptually, that would be a better solution, after Milan's remark above.

Then I tried paragraph references (ParagraphFieldItemProcessor). Its $data->getEntity() has an activeLancode 'x-default' so I get English paragraph contents, even though we just got this field ($data) from the entity that has activeLancode 'de'. (Whereas in the current code / passing NULL as the language into generate(), I correctly get German paragraph contents.) I don't even understand this.

So if we made the same change in ParagraphFieldItemProcessor, that would lead to bugs. It feels creepy to me that a reference field can work like this, so I changed my mind / want to stay away from this.

Currently running (Jobs are enabled/disabled by Drupal gitlab as needed)

OPT_IN_TEST_CURRENT:         11  
OPT_IN_TEST_MAX_PHP:         Looks to me like PHP 8.3.12 == same as CURRENT. Shrug.
OPT_IN_TEST_NEXT_MINOR:      11.x-dev
OPT_IN_TEST_PREVIOUS_MAJOR:  10
(OPT_IN_TEST_NEXT_MAJOR:     currently disabled
 OPT_IN_TEST_PREVIOUS_MINOR: currently disabled)

Merged, and back to active for the other tasks (non-urgent).

A new issue 🐛 Referenced entity in wrong language Active came in for 8.x-2.x, which proved that indeed, its Github pipeline is failing too.

I've used the draft MR there to make the pipeline succeed again, and will push it to 8.x-2.x directly.

Now that I've looked into .gitlab-ci.yml... I'll use this MR to test a bunch of other things on 3.x additionally. D10 is the most important one, but since there are a lot of other options we can easily enable: why not...

  OPT_IN_TEST_PREVIOUS_MAJOR:   ### This is D10, we definitely need this.
  OPT_IN_TEST_PREVIOUS_MINOR:  
  OPT_IN_TEST_NEXT_MINOR: 
  OPT_IN_TEST_NEXT_MAJOR:          
  OPT_IN_TEST_MAX_PHP: 

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

Also, IMHO we should make tests run on D10 too. And maybe on different PHP versions.

I don't exactly know how it works / I'm not going to look right now, but it should be easy enough; japerry did some stuff in my samlauth module → that has it running loads more versions as its standard pipeline.

This retitling implies a change of scope too, from "make Gitlab pipeline work" (though that may still be needed for 8.x-2.x) a to a broader "fuller test coverage" issue. At least to make sure that url fields and entityreference formatters have good solid test coverage, as the issue description now says.

The I'll need to go through existing test issues (or "only still open because the tests were not finished / I should recheck if all requirements are met" issues), to see if they are effectively child issues of this one now.

"Needs review" state is temporary, until the MR is committed, then it goes back to Active.

Well... when I wrote the initial spec, I assumed that "current version" would stay on D10, and D11 would stay "next major version", which would not cause the test pipeline to go red.

That wasn't true. D11 has been promoted to "current version" and tests are red now.

So I had a good look at the tests, removed the D11-incompatible dependencies, commented out relevant tests, and did a writeup in custom_elements_test_paragraphs/README.md about why this is no problem in principle... IF we get more complete test coverage elsewhere.

I'm re-titling this issue and keeping it open for that purpose / thinking about that.

Meanwhile, I'll commit the MR myself, whenever I need a pipeline to go green (e.g. to prove out that another issue is in fact OK). It contains only busywork on test(module)s.

Oops: I was too quick. Just discovered that the project type needs to be updated too. See https://github.com/drunomics/lupus-decoupled-project/pull/7 (as yet unmerged).

I've force-pushed over the first commit, which was incomplete and needed a broader solution.

This code is convoluted, arguably ugly and hard to maintain. It took me ages to structure and comment the logic in a somewhat-understandable/maintainable way and manually test/tweak the result. I still think it was necessary to spend time on it, because

• Having "Use layout builder" visible when no view mode uses it, is confusing.
• Having options invisible when in fact they do make a difference (for at least some of the view modes that use this CE display while building), is unacceptable.

I blame the convoluted-ness on the structure of configuration. (Having two separate 'layers' of displays each with their "Use Layout Builder" settings to check, combined with default/fallback displays that are used by several view modes, leads to a lot of different things to check / make clear to the user.) I see no way to make that easier and still be able to edit things when we need to.

So, it is what it is.

I'm setting this to "needs review" for a while as a sign that the MR is ready, but I'm not going to have anyone review this. It's 'only' form code, and the actual behavior is better than before.

I had counted on reusing EntityCeDisplay::getConfigDependencyEntityViewDisplays() for some of the logic I needed. But the logic turned out to not be the same, and actually this just had me doubting whether its logic is sound in the first place. So

• I added more comments to EntityCeDisplay::getConfigDependencyEntityViewDisplays() without changing the logic
• I made the method protected and removed it from the interface. This should not have any effect on classes that (theoretically) extend the interface / EntityCeDisplay. I'll publish a Change Record just for completeness. I'll comment in 🐛 Config dependencies are not properly calculated Needs work too, that my attempt at being 'smart' has failed.

This added code has no automated tests and no plans for them, "because only UI anyway". Existing tests were extended in this MR, only because I started to doubt how things worked, while coding... and the tests act as a specification of existing behavior.

I plan to merge this when I have used this myself more, in upgrading an internal project from CE 3.0.0-beta1 to this new code.

roderik → created an issue.

Thank you. Reviewed, looks good, so merged.

That's what I initially thought and why I tested. But since I saw that DDEV actually says this by itself, I personally don't think it's really necessary.

Thank you. I re-tested for your remark and think this is fine (commented why in the github PR). So I felt confident enough to merge it.

I tested the gitpod link on the project page, and it starts Drupal 11 now.

This means this issue is done, right? Just doublechecking...

---

The frontend home URL for the gitpod demo stll starts with a single "home" link (referring to itself), no link to log in, and only a message

You are not authorized to access this page.

This is what I have gotten used to, on my local dev environment, and I didn't know whether it was just my local install...

But I guess the followup to do something about it, is 📌 Add Login link toggle to demo frontend Active ...

roderik → changed the visibility of the branch 3477949-example to hidden.

@brockfanning I am mixing things up. For completeness: in my reasoning, things are going on:

1) I was expecting the account name not to be empty at the start of UserSyncEventSubscriber::onUserSync() , but filled with the $unique_id. So I still don't know where your database error about a NULL name is coming from.

Maybe the cause of this is some other hook, or maybe I am just not reading ExternalAuth code right.

But that's not of direct interest to you, at first.

2) The first interesting thing for you is that $account_data['name'] is empty at that earlier point, and that warrants checking, per my previous comment.

No, this points to

1) your "User name attribute" configuration being incorrect (or your SAML data disappearing, but I don't think so). Because $this->getAttributeByConfig('user_name_attribute') should be getting the value from the attribute named "http://schemas.xmlsoap.org/ws/2005/05/identity/claims/emailaddress", but apparently that does not exist or is empty.

(If needed, you can turn on some debugging in configuration - User - SAML authentication - SAML , to get your full IDP response including the attriubutes logged to watchdog.)

2) UserSyncEventSubscriber::onUserSync() not doing its job of giving you a proper error message.

(From reading the code, I was expecting the email to be empty and this being turned into an exception - because you're using the same attribute for name and email. I'll doublecheck this sometime when I address the linked issue - I should really make automated tests for this code.)

I would like to have the module give a more descriptive error than this, in every case. But from reading the code, I don't see how this happens.

Since the code is a maze, here's a simplified call tree:

1) SamlService::acs() calls ExternalAuth::register().

$unique_id is from the "Unique ID attribute". I don't know what this is in your case, but it is always nonempty at this point, even if the 'name' (from the SAML attribute) would not be.

2) ExternalAuth::register() does its thing and calls $account->save(). samlauth_user_presave() calls UserSyncEventSubscriber::onUserSync() just before the user is actually saved.

At this point, $account should have a nonempty name (even if it is bogus - it might contain the unique ID in theory), and it should still be isNew().

At the end of this function, the name and the email should be overwritten with the value from your user name / email attribute. (getAttributeByConfig() -> setUsername() and setEmail().)

From just reading the code, I cannot see a path that has emptied out the name and not thrown an error, at the end of this function. This is where to debug / log to watchdog / whatever.

If the username is still nonempty at the end of UserSyncEventSubscriber::onUserSync(), it's out of our hands. You have e.g. another hook_user/entity_presave that is messing things up.

(Linking other issue that might have nothing to do with it, just for a personal reminder to re-check this when I get to doing that one.)

rest_menu_items has a new allowed_menus config option, that is very likely the cause of it failing. The test needs to allow the appropriate menu first.

roderik → created an issue.

roderik → created an issue.

"example" MR is was for a live demo of Gitlab live editing - nothing to see there.

You know this, but just documenting after I did a test update: the upgrade of lupus-decoupled-project still needs D11 compatibility of

• services_env_parameter - this is on us.
• rest_menu_items → - I'm guessing they want to make the test(s) work with Gitlab at the same time.

Also, the lupus_decoupled_drupal_example module included in the repo needs D11 compatibility.

Creating MR, but I might merge this myself. Unless someone prods me, that may wait until Monday morning, while I try to get some other fixes in a 3.0.1 release.

Well... that's dumb! I was still developing on D10.2, and the "add field" button is broken on D10.3+.

Also, we don't have browser tests for the UI yet. We should, but realistically... that's not on the radar yet.

(A switch() was replaced by map() in EntityDisplayFormBase::multistepAjax(), which apparently means it cannot handle unknown values == any buttons that the Core code doesn't know about, anymore. Fine; I won't call parent::multistepAjax().)

Thanks! Merged.

The recent phpcs failures are all introduced because of the update to coder 8.3.25 → . Fixing them too.

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

roderik → changed the visibility of the branch 3469398-when-editing-a to active.

roderik → changed the visibility of the branch 3469398-remove-controller to hidden.

Thank you! Merged.

Followup created.

roderik → created an issue.

@fago summary of changes / recent MR comments:

• I was making things too complicated. The method is now only about current calculated config dependencies (method parameter was dropped).
• The 'view mode === default' case, which you said needed test coverage, is unchanged. The "non-default" code has changed.
• I do believe my way of calculating dependencies dynamically, is correct / what Drupal wants us to do. If I misunderstand, please tell me.

Given the point 3, I request a re-review, and would like to test coverage afterwards. Note that I tested manually and I'm confident of the current implementation....... provided I do not misunderstand (per point 3). I would be fine with doing this in a followup if we want to release v1 now.

(v1 would need this interface addition though.)

Hm, strange: gitlab is suddenly expecting the "use" statements to be sorted case sensitively instead of insensitively. (So the fix commit fixed things for the current CI tests, and 'broke' my current version of phpcs/coder.) I find that change surprising, but will try to understand it later. If gitlab's happy, I'm happy.

I also dropped the video paragraph/media type, per the issue description, and updated the custom_elements_thunder README.md about why. (Code we don't want to support, depending on unsupported module.)

The "composer (next major version)" build now still does not work, because media_entity_twitter (a dev dependency) has no D11 release yet. But that can have a D11 release easily / can be figured out later without having to drop code.

So: merged, thank you!

I tested with 10.2 (including the changed function definitions in the normalizer, on top of the still-old interface, just in case).

It works if we revert the renderInIsolation() change, and I would prefer that for the moment. (We can live with deprecation warnings until 3.1.)

Also: please check phpcs because these are all new. (I do not recognize some of them; maybe they are just new because of D11 inclusion / an updated coder module).

Thanks!

I couldn't merge either. (Happened to me more often.) And I seem to lack some git knowledge (about --no-ff and --squash not working together) so I can't really explain.

But I could reproduce a squashed commit locally and compare that it was equal to the branch, so all good.

@fago: this grew more complicated than I thought, and I need a review of this issue. Apologies for the boilerplate code changes that are now in here.

The complication:

• default CE displays can be used for building CEs in non-default view modes (that have no own CE display)
• If the default CE display has "use layout builder" enabled, then the 'build method' also depends on that view mode's Entity View Display (if it is enabled AND if there is no own CE display)
• I think those entity view displays should be config dependencies.

I would like to add the resulting code as a last minute addition to EntityCeDisplayInterface: getDependencyEntityViewDisplays($allSupportedModes = FALSE), because

• the default ($allSupportedModes = FALSE which returns a single display) can also be used on a 'non-default' EntityCeDisplay, to see whether its relevant 'use layout builder' option is in the specific or the default entity view display. This is non-trivial code. (Not very long, but the methods to use are strange.)
• I believe this method can replace EntityViewDisplay::collectRenderDisplays() (see MR comment)
• I have another use case for this method with $allSupportedModes = TRUE. (Finding out whether "Use layout builder" on the default CE display is relevant for any view modes, in the UI form, i.e. whether I should show the option. I'll use it in 🐛 Improve checkboxes visibility in UI Needs work .)

Review is mostly for concept / interface addition. Of course you can review all code details, but I'm not worried about it. I tested.