- Issue created by @rkoller
- 🇩🇪Germany rkoller Nürnberg, Germany
i'll also postpone this issue on 🐛 Improve the microcopy on the settings page Active . the changes in that issue make too many structural changes. i have already a working version for the changes necessary to fix this issue ready and working locally. but best would be to discuss and get the microcopy fixed first.
- Assigned to rkoller
- @rkoller opened merge request.
- 🇩🇪Germany rkoller Nürnberg, Germany
initial draft is up, i'll fix the phpcs pipeline errors now.
- 🇩🇪Germany rkoller Nürnberg, Germany
and could please credit @jurgenhaas and @drubb on this issue? the two helped clearing up my confusion in the weekly lean coffee call, moving from procedual to OOP hooks, and making sure that the legacy hooks in drupal >10.1 are working as well. on a side note i've also tested skipt in upgrade status and except the ^12 in the info.yml there were no additional changes necessary. i'Ve already added the 12 to the info,yml as well on the MR. it does no harm imho.
- 🇩🇪Germany rkoller Nürnberg, Germany
hmmm i am kind of lost and out of ideas. i've fixed the phpcs errors.. the only thing failing is the pipeline for drupal 10 ... on my local test it worked in drupal 10 before without a problem, now when i try again with the recent MR it fails with a WSOD. but if i copy in my local copy that fails as well. not sure what is causing that. i'll set the issue to needs review also for all the other changes introduced in the MR. ill unassign myself and set the issue to needs review. maybe someone else has an idea (but i keep looking alongside)
- 🇩🇪Germany rkoller Nürnberg, Germany
ok tests are green now and everything is working as expected in d11 and d10 with manual testing. just to clarify as long as a user hasnt saved their skipto settings the user is using the global settings as soon as a user is saving their settings on their user profile page the user settings are used for that user from that point on. you can still alter the global setting if you have the permissions but they have no effect to the skipto settings of your own user
- 🇩🇪Germany rkoller Nürnberg, Germany
should tests be added? the recent article by @philipnorton42 https://www.hashbangcode.com/article/drupal-11-object-oriented-hooks-and... has a few example and ideas for how to test oop hooks?
- 🇩🇪Germany rkoller Nürnberg, Germany
discussed the issue in todays weekly lean coffee call of the drupal user group munich a second time. another detail to improve is to move away from the two files (skiptoglobalsettings.php & skiptousersettings.php) the MR currently employs. that way you have redundant form definitions. the consensus the group agreed to is go go with an abstract class skiptosettings and extend it one time for global settings and another time for user settings, that way you have three files. will give it a try to improve the MR, assigned the issue to myself.
- 🇩🇪Germany rkoller Nürnberg, Germany
ok, i finally managed to get the refactoring working. instead of two separate form classes, one for global and one for the user with redundant form code we now have a single abstract class with the form and an abstract method to get the default value. in the course of the refactoring i also aligned the config and user data, now the config is also flat schema wise. manually tested on d10 and d11 and also checked with phpstan on d10 and d11 as well as phpcs on d11, everything passes now.
- 🇩🇪Germany rkoller Nürnberg, Germany
and i'Ve shortened two of the three fieldset keys. that way less multiline statements were necessary anymore.
- 🇩🇪Germany rkoller Nürnberg, Germany
and aside @jurgenhaas and @drubb please also credit @boromino who participated in this weeks discussion and initially came up with the whole abstraction approach.
- 🇩🇪Germany rkoller Nürnberg, Germany
and maybe we can come up with shorter variable name for for example $currentUser or $userData? that way we might also be able to use one liners instead of multi line statements in SkiptoSettingsUser.php (line 72 and 76).