Thanks for contributing this! Sorry it took so long.
One project is using the generic plugin, the other is using the windows_aad extension module.
For the record, we use windows_aad exclusively at work. But I did install other clients on testing sites while trying to verify the issue. They weren't functional of course. So I couldn't actually log in with them. But that raises a point I hadn't noticed before now. I don't think anyone has said when this happens. Is this a general WSoD? Or does it happen when taking certain actions like logging in or accessing admin pages?
@pfrilling for what it's worth, I can't replicate the issue where the service isn't found. I've updated a few D10 and D11 sites. They don't have any problem. I don't even have to update the database or manually clear the cache for it to work. Everything just keeps going like nothing changed before and after running the post_update function. The issue may be specific to certain situations.
I don't know how to test this. We don't rely on _user_mail_notify's return value. In fact, hardly anything in Core does either. We could do that, but we would still have to mock the function, which means we would return a mocked value. I'm not sure there's any benefit in doing that.
I'm removing the "Needs tests" tag, but if someone else comes up with an idea, then feel free to implement it. In the absence of any brilliant ideas about how to test this my only contribution will be to ensure that the tests pass with the function call added.
Good catch to see that the module wasn't enabled in that test. I didn't notice it.
With the addition of the post update function I think this is good to go.
I rebased again due to a merge conflict caused by this morning's commits to Core. This was an easier rebase than the last one, so I'm leaving it at RTBC.
I decided to just do this. It needs to happen. The module is practically undiscoverable as it is. I went with the simplest name, "Digital Analytics Program". We can change it in a follow-up issue if someone comes back with an objection.
Copying credit from 📌 Move JS library definitions into libraries.yml Active .
Superseded by 📌 Settings overhaul (for real this time) Active .
Superseded by 📌 Settings overhaul (for real this time) Active .
All the tests are green again. I'm setting this back to Needs Review instead of RTBC only because this wasn't a simple rebase this time. I had to make adjustments to the expected values in the StandardPerformanceTest. They're probably innocuous, but someone should look them over.
I'm going to have to iteratively update the StandardPerformanceTest to account for the changes to the cache as a result of the bug fix. Because for some reason my local environment doesn't want to run FunctionalJavascript tests today so that I can do this in one commit.
The test issue was merged, so this issue is unblocked.
@benjifisher Thanks for the update and link!
Just FYI: I updated the MR branch because the patch wasn't applying. It was only after the fact that I realized the patch doesn't apply to version 4.0.0. It did apply to the latest 4.0.x branch.
I use \Drupal\migrate_drupal\Plugin\migrate\source\ContentEntity a lot!
Thanks for bringing this up, @joachim. I came here to ask that it be preserved too. It's fantastic for doing data transformations within a site. And I'd look forward to not having to enable all of migrate_drupal just to have access to it.
Thanks @oily. I forgot to check on the test results after I rebased the MR to make certain it would still apply after all the recent commits to Core.
I'm not trying to be a pedant, but isn't this issue implementing the short array syntax, not removing it? I'm only asking because I opened this issue to see if there was some change in coding standards that I wasn't aware of.
Updated the target branch. Restoring RTBC status.
I have a patch mostly done.
The form elements don't need to be updated at this time. The config system automatically converts any integer values to the correct schema type. So those changes can wait until the form is overhauled.
I tried to customize the commit message to be specific about removing the obsolete settings, but it didn't work. I'm going to open individual issues for other settings-related tasks instead of continuing to use this one.
For reference, here's the list of currently-supported settings, version 8.4 at the time of this writing.
The relevant changes were committed. The last patch posted by the bot is no longer relevant because that EventSubscriber class was deleted.
I ended up doing a lot more than just requiring the agency setting for JS attachment. There was a lot of potential for clean-up in usfedgov_google_analytics_page_attachments(). Notably, I changed the check for page URLs starting with /user/ to checking the specific user.login, user.logout, and user.pass routes. I checked the original bug report for which this code was added,
🐛
Appropriate Placement of the DAP Script Tag
Fixed
. This change is in accordance with that original issue report, which specified the JS should not be attached on the user login, logout, and password reset pages.
the change is fine with me, as long as there is a good message in the release notes stating that is moved, and it can still be access from the configure link on the extend page
Understood.
This is a good time to make this change since the previous change also requires a cache clear. It will make sure this change is reflected immediately on implementing sites.
Releasing this in version 2.2.1.
The settings form class's constructor could not support both D10 and D11 as it was written.
The pattern I've implemented instead is useful for isolating a class from upstream changes. You aren't trying to override the parent's constructor anymore, so if it changes then you don't have to care or do anything, at least not in the short term. The downside is that the code is uglier because you have to add more boilerplate in the form of property declarations and setter functions.
For the record, adding the $typedConfigManager type hint resulted in this error in D10.4.1:
PHP Fatal error: Type of Drupal\usfedgov_google_analytics\Form\UsfedgovGoogleAnalyticsForm::$typedConfigManager must not be defined (as in class Drupal\Core\Form\ConfigFormBase) in /var/www/html/web/modules/contrib/usfedgov_google_analytics/src/Form/UsfedgovGoogleAnalyticsForm.php on line 15
I know there was an issue with how core implemented this new ConfigFormBase parameter. I'm not yet certain if this will cause an issue with supporting D10 and D11 with the same version.
There is no EventSubscriber\Redirect class in any version. Furthermore, the last line of that error message:
(line 40 of modules/custom/****/src/EventSubscriber/Redirect.php)
...suggests that this was an error in a custom module, not openid_connect.
Postponing this on 📌 Uncomment test cases in dataProviderForCompleteAuthorization() Active since I was able to repair those test cases.
Test case 9 had a couple of problems:
- It seemed to be trying to test two different things:
- A blocked existing user
- A mismatch in the
subproperty
The test path was not compatible with doing this. The "mismatch" case results in an early return, but then the test path expected function calls related to the blocked user. I had to make the assumption that these cases should be tested separately. It's difficult to know whether they should since there's no labels or comments on the test cases to know what the original intent was. Based on the current
completeAuthorization()code it makes sense to me that they should not be tested together. I split the "mismatch" case into a new 10th test case. - The test path was not set up to test for the
submismatch.
Set the status to NW if you want to expand the scope.
I did an investigation into whether this could be done by Core. The logic for sending emails is too intertwined with the User registration form. There's still an argument for doing it in Core, but it would be a pain.
I also checked External Auth. I didn't understand that it's mostly a helper module. Since it doesn't really do anything to check whether a user should be allowed to register, then it probably isn't the right module to be sending the emails.
That means openid_connect really ought to be responsible for sending the email.
That said, there's a problem with writing the test for this. The relevant test cases in OpenIDConnectTest::dataProviderForCompleteAuthorization() are currently commented out because of a mocking issue. This is no surprise because testCompleteAuthorization() is very complicated, which itself is a reflection of how complicated OpenIDConnect::completeAuthorization() is. So we can't really test this right now, at least not with the existing test. The test needs to be fixed, which I think means simplifying completeAuthorization() as much as possible first.
That said, if we could get the current MR merged and then add a test for it as a follow-up, then you wouldn't hear any complaint from me. Otherwise, this issue should be postponed.
I'm not sure what the problem is with the one test failure. If you remove this line:
Drupal\openid_connect\OpenIDConnect: '@openid_connect.openid_connect'
...then the test passes. But if I understand the documentation correctly, then that line is important for downstream modules to use autowiring. So it can't just be deleted.
I've tried to identify what the problem might be, but haven't had success. The error occurs while the container is being built.
The max-PHP version PHPUnit test comes back green with this MR.
@dcam can you please update the base target of this MR to 4.0.x
The target branch has been updated as requested.
Since Drupal 7 is now end-of-life I am closing some old issues for the 7.x branch of this module. If this issue is still relevant for the 3.x branch of the module, then feel free to reopen it.
Since Drupal 7 is now end-of-life I am closing some old issues for the 7.x branch of this module. If this issue is still relevant for the 3.x branch of the module, then feel free to reopen it.
Since Drupal 7 is now end-of-life I am closing some old issues for the 7.x branch of this module. If this issue is still relevant for the 3.x branch of the module, then feel free to reopen it.
Since Drupal 7 is now end-of-life I am closing some old issues for the 7.x branch of this module. If this issue is still relevant for the 3.x branch of the module, then feel free to reopen it.
Since Drupal 7 is now end-of-life I am closing some old issues for the 7.x branch of this module. If this issue is still relevant for the 3.x branch of the module, then feel free to reopen it.
Since Drupal 7 is now end-of-life I am closing some old issues for the 7.x branch of this module. If this issue is still relevant for the 3.x branch of the module, then feel free to reopen it.
Since Drupal 7 is now end-of-life I am closing some old issues for the 7.x branch of this module. If this issue is still relevant for the 3.x branch of the module, then feel free to reopen it.
Since Drupal 7 is now end-of-life I am closing some old issues for the 7.x branch of this module. If this issue is still relevant for the 3.x branch of the module, then feel free to reopen it.
Since Drupal 7 is now end-of-life I am closing some old issues for the 7.x branch of this module. If this issue is still relevant for the 3.x branch of the module, then feel free to reopen it.
Since Drupal 7 is now end-of-life I am closing some old issues for the 7.x branch of this module. If this issue is still relevant for the 3.x branch of the module, then feel free to reopen it.