Please update the existing merge request or create a new one with the relevant changes. Tests will only run against merge requests.
@ivnish This looks great; thank you!
Since this is dropping support for Drupal 9, does anyone feel that we should create a new major version of the module or are we so far past when Drupal 9 support was dropped (11/1/23) that we should not worry about that?
markdorison → made their first commit to this issue’s fork.
Tests are not currently passing or able to run and the MR is showing as unmergeable.
Hi jayapriya-18,
I appreciate your interest, but I don't see enough previous activity on your account (other than requesting maintainerships) to feel comfortable granting that to you at this time.
If others feel differently, they are welcome to pursue a different course, but in the interest of the community and security, I feel more comfortable when I see a record of contributions before granting maintainership.
Tests are failing.
Hi all, I have no problem with this request, but I do not have sufficient permissions to add you as a maintainer. Adding my +1 though.
Hi @chris. I'd love to add you but I don't have permissions to edit maintainers.
In fact, if one of the other maintainers (with the right permissions) sees this, it would be great if I could be removed as a maintainer as I don't have the capacity to keep an eye on this module.
I would recommend clearing the cache with Drush (drush cache:rebuild) as the first step as opposed to modifying the services.yml but that's just my two cents!
No problem!
Hi @Peter pulsifer! Thanks for the bug report. Can you try clearing Drupal cache and see if you still see the same error? The CsvSubscriber class was removed in
🐛
PHP 8.1 deprecated function warning
Fixed
(included in 4.0.1) so it could be an issue where the cached objects are out of date.
Thank you for the confirmations!
hestenet → credited markdorison → .
hestenet → credited markdorison → .
hestenet → credited markdorison → .
hestenet → credited markdorison → .
hestenet → credited markdorison → .
hestenet → credited markdorison → .
hestenet → credited markdorison → .
hestenet → credited markdorison → .
hestenet → credited markdorison → .
Thank you for the fix @dulnan! Is there any way we could get a new release with this included?
Happy to help!
Tests are now passing.
This failure looks like it may have been introduced in #3437544
markdorison → created an issue.
Thank you for the fixes!
Thank you for the fix!
Can anyone confirm that the changes in MR35 work with both Drupal 10 and Drupal 11?
Thank you for creating the issue. Closing as a duplicate of #3429690.
Thank you for the issue and patch! Before review, it needs to be re-rolled as a merge request so that GitLabCI tests will run against it.
Thank you for the merge request! It looks like tests are failing so moving this to needs work.
Thank you for the issue and patch! Before review, it needs to be re-rolled as a merge request so that GitLabCI tests will run against it.
This is no longer the case.
markdorison → created an issue.
The patch did not apply cleanly, but I went ahead and converted it to a merge request so that tests would run.
markdorison → made their first commit to this issue’s fork.
I didn't add the argument types out of an abundance of caution, but you're much more familiar with the module that I am.
Should be safe to add at PHP 7.4 and above! We should be good to add types throughout the module as we improve the code.
There were 2 errors:
1) Drupal\Tests\r4032login\Functional\SettingsUpdateTest::testSettingsUpdate with data set #0 (array(), true)
Behat\Mink\Exception\ResponseTextException: The text "The configuration options have been saved." was not found anywhere in the text of the current page.
/builds/project/r4032login/vendor/behat/mink/src/WebAssert.php:907
/builds/project/r4032login/vendor/behat/mink/src/WebAssert.php:293
/builds/project/r4032login/tests/src/Functional/SettingsUpdateTest.php:84
/builds/project/r4032login/vendor/phpunit/phpunit/src/Framework/TestResult.php:729
2) Drupal\Tests\r4032login\Functional\SettingsUpdateTest::testSettingsUpdate with data set #1 (array('administer r4032login'), false)
Behat\Mink\Exception\ResponseTextException: The text "The configuration options have been saved." was not found anywhere in the text of the current page.
/builds/project/r4032login/vendor/behat/mink/src/WebAssert.php:907
/builds/project/r4032login/vendor/behat/mink/src/WebAssert.php:293
/builds/project/r4032login/tests/src/Functional/SettingsUpdateTest.php:84
/builds/project/r4032login/vendor/phpunit/phpunit/src/Framework/TestResult.php:729
ERRORS!
Tests: 48, Assertions: 216, Errors: 2.@solideogloria Thank you for the issue and the MR. This looks good to me.
Merged! Thank you for the fix and to @renatog for pinging me about this.
markdorison → made their first commit to this issue’s fork.
#16 was no longer applying cleanly since the inclusion of ✨ Make support for groups optional Fixed . We need the latest changes to be in an MR so that tests will run so I created a new branch with the changes from #16 adjusted so that they would apply.
markdorison → made their first commit to this issue’s fork.
Hi yurg,
This module redirects a Drupal 403 page TO a user login page. If a user desires that functionality, this module provides it.
Hi Sunil,
Thanks for the ticket. If you have suggestions for content to be added to the README, please create a merge request.
Please ensure that the lastest changes are included in a merge request so that tests can be run. Tests are no longer run on patches since we have switched to GitLabCI → .
markdorison → made their first commit to this issue’s fork.
This looks like a good change to me. I am also curious if we could take this opportunity to add explicit argument types to the constructor arguments. I will make that change and push it up.
The PHPStan failure is related to an upstream deprecation. I have created 📌 Replace deprecated usage of League\Csv\ByteSequence::BOF_UTF8 Needs review to address that.
markdorison → created an issue.
Great idea @mglaman!
Agreed with @alexpott, @Liam Morland, and @AaronMcHale. It would be great to provide even further detail, but any additional context is a net positive here.
I am not seeing any changes in the MR 27 diff. Am I missing something?
@Ryan Thanks for the patch. Changes will need to be submitted as merge requests so that tests can run using GitLab CI. Additionally, your patch doesn't address the described issue or the proposed resolution. Moving this to "needs work."
we'd have a yaml file with raw information (will suggest a format at the end of the comment), and from this file we'd generate the constants on \Drupal\update\ProjectSecurityData (just the constants and the logic to read those obviously)
That would work, but I would advocate for replacing the constants with methods that would load the data from the YAML file directly. That way, the only thing we need to generate manually (or ideally in a build step) would be the markdown file.
Can we come up with a way to consolidate that, perhaps by refactoring ProjectSecurityData and using a script to generate the Markdown file?
What if we moved the relevant data to a YAML file? ProjectSecurityData.php could load it from there and we could create a script to modify the markdown file. I believe as things are currently configured it would still have to be run manually when those dates change. I'd love to be told I am wrong though!
LGTM!
PHPUnit tests are failing.
GitLab CI passing as expected.
markdorison → created an issue.
Hi @himanshu_jhaloya, thank you for the issue and patch! Before review, it needs to be re-rolled as a merge request so that GitLabCI tests will run against it.
Also, did you test this against a Drupal 11 install? Does the module work as expected?
markdorison → created an issue.
I am unsure about the potential implications of this change. Why wouldn't we want to rely on the more specific permissions available?
At the very least this could add confusion in scenarios where a site admin is confident that a role has not been granted the "clone content" permission(s) but suddenly now has the ability because they have the "bypass node access" permission. I am tempted to say this works as expected, but I am happy to hear feedback.
Should this be created as a pull request for acquia/cohesion instead of a patch here?
Could you update the issue title/description to provide more context for this change? This will help those trying to review.
Also, please convert this patch to a merge request so GitLab CI tests will run against it.
Thanks for the bump. 8.x-1.18 has been released with this included.
This issue was resolved in 🐛 [D10] Ludwig integration needs an update Fixed .
Created an MR from patch #3 so that tests could run.
LGTM!
markdorison → made their first commit to this issue’s fork.
LGTM!
Thanks @bakop!
I pushed a revision that I believe should accomplish the same thing, but without duplicating the entire $header array. There may be a more elegant way to do this but this was my best idea so far. Could you test to ensure this approach resolves the issue?
Needs rebase.
Rebased and resolved conflicts.