🇺🇸United States @markdorison

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.

@kksandr Thanks for clarifying that. Fantastic!

@kksandr Thank you for your work on this, especially with the new tests! Do we need to add anything test-wise for Group v3?

Rebased/updated MR so that tests will run. MR matches changes in patch #13.

markdorison → made their first commit to this issue’s fork.

Committed the changes from MR13 and I will cut a new release. Thanks for the help, and hi @inglesaaron, I am just now seeing you up there! 😁👋

All I know is this commit fixes problem with running the tests. That's what I would like to see tagged

Can you be more specific? Which tests? There are different sets of tests that run on each merge request and commit: phpcs, phpstan, phpunit, eslint, composer-lint. You can see those groups in the screenshot in #6.

This issue (and the commit you mentioned) resolved PHPStan issues to the best of my knowledge and they are passing on the 8.x-1.x branch as of the last commit. If you are seeing PHPStan errors, please create a new issue with those details and the output of the errors you are seeing.

If you are running PHPUnit tests as you mentioned in #14, those failures are detailed in 🐛 PHPUnit tests failing Needs work but assistance is welcome!

@2dareis2do PHPUnit tests are a different area than this issue/commit which has to do with PHPStan. 🐛 PHPUnit tests failing Needs work needs to be resolved for PHPUnit tests to pass.

@2dareis2do I am not against making a release, but it would be helpful for me as a maintainer to understand why a tagged release with these changes would be helpful to you.

The changes made since the last tagged release are meant to improve test feedback on Drupal.org and for contributors who have the repository cloned, but I skipped cutting a release as I didn't think they would have any effect for folks who are installing tagged releases for general use.

@2dareis2do These changes are on the default branch (8.x-1.x). Are you looking for these in a tagged release?

Hmm, I see for some reason the latest changes are not in the diff here

The changes so far in the MR are just re-enabling the tests so they will fail the test run. The work here is to resolve the two failures:

There were 2 failures:

1) Drupal\Tests\remote_stream_wrapper_widget\Functional\RemoteStreamWrapperWidgetTest::testBasicFunctionality
Failed asserting that 404 is identical to 200.

/builds/project/remote_stream_wrapper_widget/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
/builds/project/remote_stream_wrapper_widget/vendor/phpunit/phpunit/src/Framework/Constraint/IsIdentical.php:79
/builds/project/remote_stream_wrapper_widget/tests/src/Functional/RemoteStreamWrapperWidgetTest.php:106
/builds/project/remote_stream_wrapper_widget/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

2) Drupal\Tests\remote_stream_wrapper_widget\Functional\RemoteStreamWrapperWidgetTest::testRegression3043148
Failed asserting that 404 is identical to 200.

/builds/project/remote_stream_wrapper_widget/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
/builds/project/remote_stream_wrapper_widget/vendor/phpunit/phpunit/src/Framework/Constraint/IsIdentical.php:79
/builds/project/remote_stream_wrapper_widget/tests/src/Functional/RemoteStreamWrapperWidgetTest.php:125
/builds/project/remote_stream_wrapper_widget/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

FAILURES!
Tests: 2, Assertions: 18, Failures: 2.

It does not even matter the project has been already opted into security advisory policy.

This was not clear to me. Thank you for clarifying!

@apaderno I can vouch for @apotek's qualifications to receive the "opt projects into security advisory" permission. We have collaborated on Drupal code for many years. A recent example would be the Orange DAM → module where he has written a substantial amount of code.

We don't currently have a new project to submit for security advisory coverage → since I opted Orange DAM in myself. If this is not sufficient, please advise the best way to proceed.

I added support for group version 1 in merge request #20. That is, it now supports all versions of groups.

Does anyone have any ideas on how to cover this with tests? That is, run tests with different versions of groups.

Thank you @kksandr! In that case, I agree, let's turn our attention to MR 20.

OK, makes sense to me!

The Automated Testing tab currently serves two purposes related to GitLabCI:

1. Copy informing people about GitLab CI's existence and the fact that it is the preferred method of testing
2. A link to documentation detailing how to configure it → .

If we remove this tab/page, what is the best alternative to get this knowledge in front of maintainers/contributors?

This is a complicated problem to solve elegantly.

Both 2.0 and 3.0 are currently supported Group versions and will be for some time -- or such is my understanding

Given this, I am inclined to prioritize reviewing and merging support for 2.x (MR17) and then circle back to 3.x maybe with a new issue. All that to say, please review and provide feedback on MR 17.

Could we please convert this work to a merge request so that GitLabCI will run against it?

PHPCS is flagging these additions because ManageVersionGroup is not used in those changed files.

I noticed that 'use Drupal\quick_node_clone\ManageVersionGroup;' is missing in the following files:

What led you to those specific files as needing the use statements?

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.

Please submit any changes via the current or a new merge request so that GitLabCI tests will run against it. Thank you!

I am planning to commit the changes in MR 13 and create a new release. That said, these changes drop support for Drupal < 9.2. Since all of the versions we are dropping have already reached end-of-life, I don't feel it's necessary to create a new major version of the module but I wanted to share these intentions here first in case anyone would like to weigh in in one direction or another.

Please re-roll this change in a merge request so that GitLabCI tests will run against it. Thank you!

Please update the merge request so that GitLabCI tests will run against it. Thank you!

I created an MR from the patch so tests could run. Resolved a small PHPCS issue. Committing this now. Thanks for the ping!

markdorison → made their first commit to this issue’s fork.

• This change will require the dropping of support for versions of Drupal prior to 9.2
• Please re-roll this change in a merge request so that GitLabCI tests will run against it.

Also, I updated the issue description with links to the supporting docs.

markdorison → made their first commit to this issue’s fork.

Please re-roll this change in a merge request so that GitLabCI tests will run against it. Thank you!

GitLab CI checks passing:

markdorison → created an issue.

Working to get tests passing.

Tugboat preview building successfully.

markdorison → created an issue.

markdorison → created an issue.

I just left a comment with an idea on how to not drop support for versions < 10.2 and there is a different idea presented in #7. Marking this as needs work so we can see what else we can come up with.