Remove debugging cruft from shibboleth.install

[jvogtuw]

The code in shibboleth_requirements() is leftover from debugging and isn't needed. It also logs confusing messages to the Watchdog log. Remove the if statement

[rbruch]

After looking at this some more, I'm wondering if we need the function shibboleth_requirements at all, since, once we remove the if statement, the function won't do anything but return an empty array.

I've tried this out by removing the whole function and the module seems to continue to work for logins/logouts. I did not test install/uninstall, however.

What do you think? Is it important to leave a stub function for hook_requirements, or can we just omit it?

[jvogtuw]

It's a good question. I think it's probably fine, but we should test install/uninstall before we remove it entirely. I'll need to read up on that hook and remind myself what it does

[rbruch]

I looked into this a bit more... I installed Drush (11.4) on my dev server (running Drupal 10). I then used the generator to generate a test module skeleton. I found that if I elect to have the generator create a .install file, it includes hook_install, hook_uninstall, and hook_requirements functions that are exactly the same as what we have here in the shibboleth module. The fact that the creation of a .install file is optional makes me think we could possibly do away with the file altogether.

Alternatively, I tested uninstalling and reinstalling the module with the existing shibboleth.install file which has the shibboleth_requirements function removed entirely, and it worked as expected. The various settings for the module were removed on uninstall, and when I reinstalled and reconfigured, the module works fine.

So it seems to me that there are three options, in order of "ambitiousness": 1) Remove the .install file altogether, or 2) just remove shibboleth_requirements function from the file, or 3) leave the shibboleth_requirements function, but remove the if statement block.

Disclaimer again that I'm in uncharted waters at this point.

Also, here's the link to the API docs for the relevant hooks for reference: https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Extension!module.api.php/function/hook_requirements/10

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Extension%21module.api.php/function/hook_install/9.3.x

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Extension%21module.api.php/function/hook_uninstall/10

[jvogtuw]

Oh, awesome! Thanks for researching this. I would opt for #2 although I don’t have a good argument as to why. I think it just feels weird not to have the .install file. What do you think?

JEANNA VOGT Senior Web Developer Business Innovation & Technology UW Facilitieshttps://facilities.uw.edu/ she/her 206.221.6033 @.**@.> web requests/support: @.**@.>

From: rbruch @.> Sent: Thursday, January 26, 2023 3:31 PM To: jvogtuw/shibboleth @.> Cc: Jeanna Vogt @.>; Author @.> Subject: Re: [jvogtuw/shibboleth] Remove debugging cruft from shibboleth.install (Issue #4)

I looked into this a bit more... I installed Drush (11.4) on my dev server (running Drupal 10). I then used the generator to generate a test module skeleton. I found that if I elect to have the generator create a .install file, it includes hook_install, hook_uninstall, and hook_requirements functions that are exactly the same as what we have here in the shibboleth module. The fact that the creation of a .install file is optional makes me think we could possibly do away with the file altogether.

Alternatively, I tested uninstalling and reinstalling the module with the existing shibboleth.install file which has the shibboleth_requirements function removed entirely, and it worked as expected. The various settings for the module were removed on uninstall, and when I reinstalled and reconfigured, the module works fine.

So it seems to me that there are three options, in order of "ambitiousness": 1) Remove the .install file altogether, or 2) just remove shibboleth_requirements function from the file, or 3) leave the shibboleth_requirements function, but remove the if statement block.

Disclaimer again that I'm in uncharted waters at this point.

Also, here's the link to the API docs for the relevant hooks for reference: https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Extension!module.api.php/function/hook_requirements/10https://urldefense.com/v3/__https:/api.drupal.org/api/drupal/core!lib!Drupal!Core!Extension!module.api.php/function/hook_requirements/10__;!!K-Hz7m0Vt54!kEWQxs7FZgQoY2dp3k3_iqb9UvekaEOyDCXARW52j9iiOuk_gH9anfhH6Izto-mNm66k1-ADqyHA3DAPDQUhAw$

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Extension%21module.api.php/function/hook_install/9.3.xhttps://urldefense.com/v3/__https:/api.drupal.org/api/drupal/core*21lib*21Drupal*21Core*21Extension*21module.api.php/function/hook_install/9.3.x__;JSUlJSU!!K-Hz7m0Vt54!kEWQxs7FZgQoY2dp3k3_iqb9UvekaEOyDCXARW52j9iiOuk_gH9anfhH6Izto-mNm66k1-ADqyHA3DCi0rQeAA$

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Extension%21module.api.php/function/hook_uninstall/10https://urldefense.com/v3/__https:/api.drupal.org/api/drupal/core*21lib*21Drupal*21Core*21Extension*21module.api.php/function/hook_uninstall/10__;JSUlJSU!!K-Hz7m0Vt54!kEWQxs7FZgQoY2dp3k3_iqb9UvekaEOyDCXARW52j9iiOuk_gH9anfhH6Izto-mNm66k1-ADqyHA3DAzhw_QQQ$

— Reply to this email directly, view it on GitHubhttps://urldefense.com/v3/__https:/github.com/jvogtuw/shibboleth/issues/4*issuecomment-1405811898__;Iw!!K-Hz7m0Vt54!kEWQxs7FZgQoY2dp3k3_iqb9UvekaEOyDCXARW52j9iiOuk_gH9anfhH6Izto-mNm66k1-ADqyHA3DCX-qYmbw$, or unsubscribehttps://urldefense.com/v3/__https:/github.com/notifications/unsubscribe-auth/ADVILOFAY2PTD5NYGB6WWLLWUMCMRANCNFSM6AAAAAAUEMAKAI__;!!K-Hz7m0Vt54!kEWQxs7FZgQoY2dp3k3_iqb9UvekaEOyDCXARW52j9iiOuk_gH9anfhH6Izto-mNm66k1-ADqyHA3DBBxu0UzQ$. You are receiving this because you authored the thread.Message ID: @.***>

[rbruch]

I encountered a wrinkle this week: at some point, my site started showing an error, part of which said there was "Mismatched entity and/or field definitions" referring to the shibboleth authname field.

I think the error started appearing after I updated a contrib module using composer "with all dependencies", which also updated some Symfony files. I did also run the update.php script after that.

I'm really not sure what triggered the problem; if it was just the uninstall/reinstall that I did, or the uninstall/reinstall plus the module and Symphony updates along with the update.php run.

I'm also not sure exactly what the problem was, but was able to resolve the error message by doing the "go to the field and save the settings" trick described in this thread: https://www.drupal.org/forum/support/upgrading-drupal/2019-06-23/how-to-solve-mismatched-entity-andor-field-definitions

So, all that said, since something happened that I can't explain, I'm inclined to go conservative with any changes to the .install file. Either option 2 or even 3 above seem safest to me at this time.

Finally, I'm not sure if we should maybe create a new issue for the mismatched field definition error? I'm not sure if I could provide a reliable "steps to reproduce" description though and I would not know where to start tracking it down...

[jvogtuw]

Thanks for all your help on this module and I hope you enjoy your non-UW endeavors!

I'm going to push a commit that removes shibboleth_requirements() and shibboleth_path_requirements() and I'll close this issue with it. If others experience the entity field definition mismatch errors in the future and can provide the steps to reproduce, let's start a new issue.