🇮🇪Ireland @lostcarpark

Change now merged. Thank you all for working on this.

I have reviewed the test changes, and I'm happy they properly test the redirect path validation.

I have run the tests locally, and carried out a manual test.

This change is now ready to merge.

jdleonard → credited lostcarpark → .

jdleonard → credited lostcarpark → .

Hope it's appropriate for me to open a MR.

Apologies, but I should have thought of this in the original issue description.

I think test coverage is needed to ensure the validation is working.

In the VerifyEmailSetupTest::testVerifyEmailConfig, the test assumes that the save of the verify email form was successful. However, this is no longer a valid assumption since it could fail validation.

• After the first submit on line 65, we should use addressEquals to verify the form saved and returned to the /admin/people/verify-email page.
• We should also statusMessageContains to check the status message is correct.
• We can remove the drupalGet on line 66 since the page should have redirected there anyway.

We should also add a new test function in this class to test that an invalid destination path won't save.

• This should attempt to save a new verify email form, but specifically put a path that doesn't exist in the destination path
• It should assert with addressEquals remains on the add page.
• It should assert with statusMessageContains that the correct error message is generated.

Thank you for your work on this.

lostcarpark → created an issue.

Just adding tag for DrupalDevDays in Leuven. This opened after discussion with @alexpott on security issues, coded by dhruv.mittal, reviewed by nidhi27, and merged during the event.

Merged successfully.

I have tested manually, including the update hook.

Tests are passing, and code looks good.

Moving to RTBC.

Change merged.

Thanks @dhruv.mittal and @nidhi27 for contributing to this issue.

I have tested manually too, and everything is looking good.

Merging now.

I didn't realise the update hook ID must be above 8000. I'm suggesting we start from 10001 since we have no connection to Drupal 8.

Also, we should add a type hint for the entity so that properties and methods can be validated.

Hopefully that will be the very last thing before we merge.

@dhruv.mittal, are you still working on this?

If you are, please comment. If not, please unassign so others can work on it.

Thank you for your contribution.

Please just update the number in the update hook, and we will be good to merge.

Thanks for your work!

lostcarpark → created an issue.

This change is almost ready, but just needs finishing off.

All the necessary changes are in VerifyEmailAddEditForm:

1. In the path field move the "description" under "title", and "leave empty to use the front page" to "use for front page."
2. In redirect_path remove the "#field_prefix".
3. Also in redirect_path, add a "description" property with text of "The internal path the user will be redirected, including leading slash, or to redirect to front page of site."

Updated version of NA banner with new #first-contribution Slack channel:

Thanks everyone for contributing.
@kul.pratap nice work on the fix.
@kul.pratap good catch on the namespace.
@sagartiwari thank you for the review. The screenshots are nice, but slightly overkill. It's usually enough to say you tested the change, and have reviewed the code.
Merged #11 and moving to fixed.

lostcarpark → created an issue.

jdleonard → credited lostcarpark → .

Change merged.

Thanks for working on this.

This looks good. I have tested manually, and verified it behaves as expected.

Moving to RTBC.

lostcarpark → created an issue.

This is almost perfect. There are just a couple of issues I'd like to tidy up.

Update title and redirect method.

lostcarpark → created an issue.

Hi,

Confirming I'd be happy to be added.

I would be happy to help lead mentoring meetings.

lostcarpark → created an issue.

Thanks for the review!

Changed the "Create a patch" to "Create a merge request", and provided links to documentation on using GitLab for contribution, as patch files are no longer recommended.

Change merged.

Thanks for your work on this.

Excellent work. I've just flagged some minor issues that need correcting.

@seanpb Thanks for reporting this, and providing the fix. However, it still needs a merge request.

Is there a way we could add a test to check for the JS error to ensure I don't unwittingly break it again?

I forgot to set back to needs work in my previous comment.

lostcarpark → created an issue.

Change merged. Thanks for your help!

Confirmed the cache flushes have been removed, and tests pass.

I have also tested manually, and run the tests locally, and confirmed all pass.

Great work on this, thanks!

This looks great and seems to solve the problem.

To get the tests to pass, I had to add the following line in two places:

drupal_flush_all_caches();

In line 76 of VerifyEmailRouteTest, and line 62 of VerifyEmailSetupTest.

I think these can be removed, and providing the tests pass, we should be ready to merge.

lostcarpark → created an issue.

I'm not going to have time for a couple of weeks, but I will review as soon as I can.

If anyone else has time to look at it before then, please go ahead.

Change merged.

Thanks dhruv.mittal and nidhi27 for your assistance.

I have reviewed the change, and everything looks good. I've also tested manually, and run the tests locally. The pipelines are all green, so let's move to RTBC.

I think we're almost there. I just think the name of the controller is a little unwieldy. I think we can shorten that and we should be ready to merge.

Tests are currently failing. The problem is that we are still passing a parameter to the controller called `$entity_id`, but the controller is expecting it to be called `$entity`. We need to update the parameter name to correct this.

jdleonard → credited lostcarpark → .

Crediting cmlara for assistance via Slack.

jdleonard → credited lostcarpark → .

Thank you ankit_k and nidhi27.

Change is now merged and issue fixed.

Thank you both for working on this. This looks good and all tests are now passing.

I have reviewed the change, checked the tests pass locally, and carried out a manual test, and I'm happy to move to RTBC.

Thanks for working on this, @ankit_k.

Tests are failing because VerifyEmailRepository references VerifyEmailInterface, which was in the same namespace, so didn't need a use statement. To fix, add use Drupal\verify_email\VerifyEmailInterface; to service and its interface.

PHPCS is also failing for the following reasons:

In Verifier, use Drupal\verify_email\Service\VerifyEmailRepositoryInterface; is no longer required, because both services are in the same namespace.

In VerifyEmailRepositoryInterface, the @return docblock needs the full path to the class (again, it wasn't needed before because it was in the same namespace): @return \Drupal\verify_email\VerifyEmailInterface|null.

Again, thanks for your efforts. I think it only needs a couple of minor fixes to get over the line.

Thanks for working on this. It looks great, but there are a few small points.

The test is currently failing because the page title is actually "Thank you | Drupal". However, I think it's better to check the <h1> text.

I've also made some other suggestions, mainly around passing the entity into the controller parameter instead of the entity ID.

Finally, there are some PHPCS errors that should be fixed.

Thank you for submitting the fix. However, you don't appear to have opened a merge request. I'd be grateful if you could open one.

What is the reasoning behind removing the autowire form the legacy hooks? I was under the impression autowiring was needed for backwards compatibility of legacy hooks.

lostcarpark → created an issue.

A problem for this issue is that while the VerifyEmail entity was available in the form build, it was not available in the submit method.

Since 📌 Create service for email verification Active is completed, the entity is stored in $form_state, so is available in the rest of the form.

I will add some additional detail to the issue.

@dhruv.mittal are you still working on this?

Alpha 1 release created.

Merged change.

lostcarpark → created an issue.

I was worried about the link being vulnerable to brute force attacks, so I added a second "secret" parameter to the route, and generate a random of string of characters. This is stored in in TempStore, and must match when the link is clicked.

I have added the following test cases:

• Test that entering a valid email address submits the form and displays a message
• Test an invalid link (not stored in TempStore) does not log in
• Test a link with incorrect secret does not log in
• Test an expired link does not log in, but does redirect to verify form and ask to reenter email
• Test a valid link does log in and redirects to the destination page

Edited issue as the way I thought it would be implemented turned out not to be practical.

I have created the Verifier service.

The basic functionality to send the verify email, and create and log in users now exists.

Some test cases are still required.

I think the Tim's suggestion seems overkill for this, and I don't think a FunctionalJavascript test is required.

My thinking is the test should place a block with pagination enabled, and verify the result count is displayed. Then edit the block, disable pagination, and verify result count not shown.

There are some existing Functional tests in the file ProjectBrowserBlockTest.php that should be very similar, and it should be very straightforward to add a very similar test.

lostcarpark → created an issue.

lostcarpark → created an issue.

Steps to reproduce:

1. Install Drupal and Project Browser module.
2. Go to "Structure->Block Layout"
3. Find the "Content" block and select "Place Block"
4. Scroll down the list of blocks and find "Contrib Modules" and "Recipes" in the Project Browser category
5. Click "Place block" button next to "Recipes"
6. In the block settings, uncheck "Pagination", then save
7. Go to front page of site and view mini browser block

The browser will show "12 results" above the list of recipes

lostcarpark → created an issue.

Saving the entity attribute that can't currently be used without breaking D10 compatibility.

Postponing until D12 releases, and D10 is end of life.

lostcarpark → created an issue.

Change merged. Thank you for working on this, @nidhi27 and @keshav patel.

I have run the tests locally, and carried out manual testing, and this looks good to me. Moving to RTBC.

I hadn't expected this to start before !3 merged, so it is impacted by some of the changes in that.

I have rebased to bring those changes in.

I think the check itself should be fine, but the test will need updating.

Thank you for working on this @nidhi27. I've made some minor suggestions around streamlining the constructor and reducing booilerplate in PHP 8.

Change merged.

lostcarpark → created an issue.

lostcarpark → created an issue.

!2 merged.

I have added a "verify email" config entity.

I was hoping to use PHP attributes to define the entity. However, this is only supported from Drupal 11.1. As I need to support D10, at least for the time being, I have converted the attributes back to a comment annotation.

The config form allows adding, editing, and deleting of entities.

The fields of the entity are as listed above.

I have written a test to add, edit, and delete a "verify email" entity, and it is passing for all versions GitLab CI tests against.

I think this is ready to merge.

mradcliffe → credited lostcarpark → .

Agree this is beyond a rebase. Needs a new branch from 2.0, and for the changes to be reworked to match.

lostcarpark → changed the visibility of the branch 3513851-add-config-entity to hidden.

lostcarpark → created an issue.

Merged initial module files. Will make useful in follow up issues.

lostcarpark → created an issue.

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

mradcliffe → credited lostcarpark → .

Marking needs review for @chrisfromredfin. I know there's still more work needed, but I would like some feedback on what's been done.

Currently the help button is added to all multiselect drop-downs. Would be nice to make it only shown if the filter has help text. And maybe make it more general, so any filter can offer help text.

Also needs some tidying up to make the tests work again, but would be helpful to get some feedback on whether this takes it in the right direction.

Merged !22.

Merged !23.

Rebased and tests are now passing.

I agree TruncateHTML should be a service. I'll check if there's an issue, and open one if not.

I've fixed the issue with the invalid permission being added.

I also noticed that the permission for the Tools menu was only the module's own permissions, but that wouldn't allow access to other functions such as run database updates, or devel menu options. I'm also aiming to have other options under the tools menu if other modules are enabled in future. For now I'm making the Tools menu available to anyone with access navigation permission. We can look at tightening that up in a follow up issue.

Also some improvements to tests.

Thanks. I missed that, and somehow didn't notice there was an update hook.
I'll revise the hook to add the new permissions, though I'm in two minds about whether an update hook is needed at all. Will make sure to include in release notes so admins can adjust their permissions if required.
As a side note, I'm switching to object oriented hooks, but as far as I know, update hooks have to remain procedural (for now).

Merged into 1.1 branch. I will get a release out soon.

Rebased to 1.1.x.