Updating to v2.0.4 causes authmap view to produce an unexpected error

Automatically closed - issue fixed for 2 weeks with no activity.

Same error with 2.0.5. Only downgrade to 2.0.3 solve problem.

The issue is back after 2.0.5, I can also confirm.

That's weird.

Can you check your database, whether you have records in the authmap table that do not have a numeric value for the `uid` column?
E.g. by running the SQL query SELECT count(*) FROM authmap WHERE uid = ""; and getting a result other than 0?

I ran into almost this same exact issue using the samlauth module. There is a merge request with a working fix available in that issue that might help resolve this issue. See https://www.drupal.org/project/samlauth/issues/3424834 🐛 Error encountered when accessing SAML authentication mapping configuration page Active .

Observations from a quick test:

• In 2.0.3, the UID is present in two properties: $row->authmap_uid and $row->users_field_data_authmap_uid
• In 2.0.4/5, the UID is present in two properties: $row->uid and $row->users_field_data_authmap_uid
• I did not see errors in any version. Also not when downgrading.

Working theory:

• authmap_uid changed to uid because of the changes to hook_views_data in 2.0.4.
• In hindsight, I didn't delete / re-import the view after updating to 2.0.5. Maybe users only see the error when they 'newly import' the view on 2.0.5? (Or maybe only when they do not do that... if I made some testing mistake?)
• Then again... After looking at the changes @svendecabooter made in his commit linked here (that is part of 2.0.5), It seems very strange to me that anyone would still see the reported error. (A blank field: ...sure, maybe. But not the error. The error would only be present in... the dedicated view that samlauth ships.)
• The two fields in the row are for "the base field" and "the uid field" respectively.
• I guess using "the uid field" for the link is just as good / better, conceptually, than using "the base field". (Which I didn't think of, when initially coding this thing.)
• From that perspective, switching to $row->users_field_data_authmap_uid might be better - especially since it sidesteps the change introduced here.

Also I'm not really sure I agree that 'uid' should be defined 'the base field' in views data, because it's not unique?

But I don't know all this for sure yet; I'm going to postpone looking at the views code (and any possible effects of the views data change on samlauth) to $some_time when I have more time. So I'm also probably not going to commit the samlauth fix until that time. At that point, I'll likely incorporate the 'fortification' that was committed to 2.0.5 here.

(This is just a braindump so I don't forget the details until then.)

Thanks for the analysis roderik.
I have updated the logic for the delete link, to not render if $row->uid is not set.
So the only reason why it would still trigger an error, is if that value is set, but to an empty string for example.

I created a merge request that more thoroughly checks if $row->uid actually has a valid value, and also provides a fallback to the $row->users_field_data_authmap_uid value, since that is indeed more consistent among different module releases, from what I can see.

Can people experience this issue try out this MR fork and see if it resolves the issue for them?
If not, can you double check you are testing the View provided by ExternalAuth, not by the samlauth module?

I'm getting the same error using v2.0.6 and I'm using saml authentication 8.x-3.10 (as of this posting it's the latest version).

Couple of updates:

• catering for all known uid properties
• throwing exception instead of returning NULL as parent doesn't cater for NULL

tested

good to go!

I have provided some feedback to the MR.

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.

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 .

Changes in MR look good to me... I've merged these in. Thanks for the updates!

Automatically closed - issue fixed for 2 weeks with no activity.

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.