🇮🇪Ireland @lostcarpark

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.

I've done some manual testing and this looks good to me.

Merged.

There are a couple of warnings about deprecations for next minor/max PHP, but we'll take care of those in separate issues.

@Jurgan, you make a good point about depending on the internal Navigation internal library. I think it would be worth looking at better ways to do that, but I think it should be in a follow-up issue.

Thanks for raising this. Clearly the library should only be loaded when the navigation is loaded, and certainly not for anonymous users.

Merging.

I've converted the module's only hook to an object oriented one with legacy fallback.

Tests are producing a warning for next minor because of deprecations in the Devel module. I don't think I can do anything about this other than wait for Devel to fix the issue. That warning is not resulting from this change.

lostcarpark → created an issue.

The PHPStan failure should be resolved by 🐛 PHPStan fails for Next Minor Active , at which point this can be rebased cleanly.

Changing return type of `TruncateHTML::init` from `DOMDocument` to `DOMNode` resolved the issue.

the `init()` method calls `HTML::load()` it would seem that this was always returning a `DOMNode`, but hadn't specified return type, so it wasn't caught.

Ready for review.

lostcarpark → created an issue.

I have created a SmartTrimHooks service and moved the current hooks into it. I added legacy hooks for backwards compatibility.

I had to use ContainerBuilder in the unit test setup, as I was getting an error due to container not being initialised.

I am getting a PHPStan warning for "next minor". I don't think it's related to this change, so I'll open a separate issue to address it. We might want to address that, then rebase this with it's merged.

lostcarpark → created an issue.

I did some testing and found it worked well. It took about 6 minutes to spin up, which is pretty good going. I like the way it gives you a control panel that lets you launch the site and the VSCode environment separately. I found the tiny site window in the corner of the old DrupalPod less useful.

I haven't tried pushing to a repo yet. I'll try that and post when I do.

mradcliffe → credited lostcarpark → .

I'd be happy to help with review too.

I have added test cases for block access.

testProjectBrowserBlockAccessWhenDisabled tests the black is initially visible, then updates the project browser settings so no sources are enabled, and verifies block is no longer visible. Is the $this->rebuild() after the settings change correct?

testProjectBrowserBlockAccessNonPrivileged tests that the block is visible for the admin user, then creates an unprivileged user, and checks it is not visible for them.

I think I've taken care of these now.

I think I've made the changes suggested by @phenaproxima. Tests are passing, though I think we will need more tests on the browser block.

Moving to needs review to see what comes out of it.

lostcarpark → changed the visibility of the branch 3450629-mini-browser to hidden.

Think I have most of the changes needed done. I'm having a problem with the test (renamed to ProjectBrowserBlockTest), which passes when I run locally, but fails in the CI.

@phenaproxima I branched off the current 2.0.x, so I think the current code is good. I made mave missed a step because it's an old fork.

I think it should be straightforward to strip out the parts that don't fit in the new scope. I will aim to get that done soon.

Resurrecting this issue, I've created a new branch.

As discussed previously, I've added a button next to the category dropdown:

Pressing it shows a pop-up with the category descriptions:

To do:

• 3 tests in ProjectBrowserUiTestJsonApi.php are failing. I'm struggling to figure out the cause, as they are passing locally.
• Currently the header of the pop-up is not translatable. I meant to fix that before pushing, but forgot.
• There probably should be some explanatory text before the descriptions.
• At the moment the button just has a "?" for the label. There should probably be an aria label for it. I'm not sure if there's a way to tell screen readers not to read the question mark, as it's not very helpful.

lostcarpark → changed the visibility of the branch 3293907-provide-brief-summaries to hidden.

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

Changed Clear Filters button to also clear the search text.

Added testClearSearch to text both "X" on search box and "Clear Filters" clear the search text.

Verified tests pass.

The generic filter sounds good, but transferring the work I did on ✨ Make the overall design of the browse page more compact and dense Active in here.

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

Well, doh! Didn't notice this has been merged and tried to add test.

Will add in follow-on issue.

I'm eagerly awaiting this issue to be ready for testing. I managed to patch Olivero to create a subtheme back under D9, but the patch wouldn't work for me in D10.

As a stop-gap, I've been using Jean Valverde's Olivero Subtheme Project. It links to this issue, and warns that it will soon be obsolete, but it doesn't seem to be quite yet!

I fully expect to recreate my subtheme as a Starterkit theme when possible, but just now Jean's project seems to be the best option for Olivero derived themes.

The change in the MR seems overcomplicated, and I'm not sure it fixes the problem that the module won't install if there's a Markdown filter already existing.

Wouldn't a much simpler solution be to move config/install/filter.format.markdown.yml to config/optional/filter.format.markdown.yml?

This would mean that if there's a existing Markdown filter, it would be left as-is, and it would be up to the site builder to apply Markdown Easy to it. I feel this is reasonable as we don't know what might be in an existing filter, so we shouldn't change it. I think expecting a manual reconfiguration of the text filter is reasonable.

But there is. and has been at least since Jan 13, 2022.

Can you point to the specific support for GitLab Flavor Markdown? I don't see anything in either CommonMark's build in extensions, or in community extensions.

Is it in CommonMark or another Markdown library? @ultimike has said he doesn't want to get into supporting multiple libraries, since that is one of the things that makes the Markdown module difficult to use.

I think the change looks good.

I did a manual test and verified the Markdown Smörgåsbord flavor was available. When selected, I was able to use the footnote markdown. Switching back the GitLab flavor caused the footnotes to not render, as expected.

[ Note, it would appear that GitHub does now support footnotes. However, I think you stick to the GitHub markdown provided by Common Markdown. It might be worth opening an issue for that library, though. ]

I like the implementation of the new hook.

As the hook change is a breaking change, should you consider a major version change (to 2.0.0) when releasing this?

Moving to RTBC.

Sorry, will get reviewed today.

I thought this would need a Svelte code change, but when I pushed that change it caused a test to fail. Looking at the fail, I needed to move a container class back, which was when I realised the Svelte change wasn't needed and it could all be done with CSS.

I've moved the Search box inside the grey filter area. I think this connects them better visually, and allows the spacing to be tightened up:

I also checked it doesn't break in mobile view:

I think there would be scope for a more radical change, but after discussing in Slack, there wasn't an appetite for that, so I've kept it to simple tightening of the spacing.

The class gets added to the <a> link here, and as @lrwebks says above, then gets passed to the template as #more_wrapper_class.

The class is then added to the wrapper <div> here.

It definitely doesn't seem right that the same class gets added to both. A workaround for sites using Smart Trim would be to override the template, and only use $more_settings['class'] to set the class of the <a> link, and set a different class for the <div> in the overridden template.

I do think we want to avoid adding too many additional paramaters, as providing the template allows the output to be modified in more flexible ways.

I'm wondering should we apply a class to the <a> link at all, since it can be pretty easily be specified in CSS by .class-name a, but as @anybody says, that could be a breaking change for sites relying on it.

Agree we should wait for input from @markie or @ultimike.

roderik → credited lostcarpark → .