🇮🇪Ireland @lostcarpark

I've carried out a manual test, and this change is now working well for me.

I think the README addition covers it well, though I think some extra detail in the documentation page might be helpful.

Moving to RTBC.

jdleonard → credited lostcarpark → .

I have converted hooks to the new object oriented style.

The only functions remaining in the .module file are the template preprocess functions. Not sure if it's possible to replace them and remove the .module file altogether.

I also updated tests making direct calls to hook functions.

All tests are passing, and I've carried out some manual testing. Not sure if there are any gaps in our test coverage. I'm sure my manual tests didn't cover all the functionality provided by the hooks.

Appreciate if anyone can review.

I tried converting User related hooks as the first step, and all went smoothly.

lostcarpark → created an issue.

Thanks for finishing this off. Sorry I didn't get it all done - it's been one of those weeks!

Thanks for the feedback!

I will have some time to work on this next week, so will tense so it can be applied to current releases, and hopefully get some more entities sorted.

Thank you both for working on this. I'll do some final checks and merge.

Some updates from last month.

lostcarpark → created an issue.

I converted UserContact to attributes.

I encountered this issue.

 *     "local_task_provider" = {
 *       "default" = "\Drupal\entity\Menu\DefaultEntityLocalTaskProvider",
 *     },

I couldn't find this in core, and eventually tracked it down to the Entity API module, which isn't a dependency of CRM.

After discussing with @bluegeek9, removed above section from the attributes.

Tests all pass, but will need additional testing to make sure it isn't needed.

The same string was repeated 5 times in AddEditFormatTest.php, so I made a small change to add constants for the messages relating to expected tags in HTML filter. I think this makes it tidier, and hopefully clearer what each message relates to.

The rest looks good to me, but I'll carry out some additional testing.

I have converted ContactDetailType to use attributes. I picked it for the first one because I thought a config entity might be slightly easier than a content one.

Tests are passing for current and future versions.

As expected, tests fail for previous minor and previous major. I believe GitlabCI is still running on 11.1. It should soon be moving to 11.2, so we should be able to test against previous minor again soon. We will need to turn off previous major until D12.

I have reviewed the change, and checked the codebase for any other duplicate instances of <dl> <dt> <dd>, and didn't find any.

I did not carry out a manual test, but I don't think it's required.

I think this is ready for RTBC.

Added a library with CSS classes for left, centre, and right table cells.

It seems a waste to always load this, since it will only be required by a minority of markdown that makes use of tables.

To prevent this, I have added a regex check for table cells with relevant classes, in the converted text, and only load the library if needed.

I check for the presence of the CSS file, and table cells in FilterTest.

Ready for review.

At Member Platform Zoom meeting, 3 Jul, the issue was discussed, and most agreed that it is better to comply with latest standards than support all possible versions of Drupal.

It was also mentioned that CRM module is targeting a stable release for DrupalCon Chicago, which should be only a few months from D10 end of life.

On this basis, I'm going to continue working on converting entity annotations to attributes, and bump the minimum supported version to 11.1.

jdleonard → credited lostcarpark → .

jdleonard → credited lostcarpark → .

To start this off, I have converted the annotations to attributes for plugins.

I'm leaving entities until we decide what our minimum supported Drupal version is.

Before proceeding we need to decide our target minimum version:

• Drupal 10.2: Supports attributes for Blocks and Actions (see CR 3395575 → ). I don't think either are currently used in CRM.
• Drupal 10.3: Adds attributes for a long list of objects, including Constraint, ViewsField, ViewsQuery used by CRM. See CR 3229001 → for the full list.
• Drupal 11.1: Adds attributes for ContentEntity and ConfigEntity. These are the bulk of annotations in CRM. See CR 3505422 → .

So if we need to support D10, we can only convert plugin attributes for now. We may wish to maintain support for D10 until its end of life in summer 2026. We would need to increase the minimum supported version to 10.3, but earlier versions are out of support anyway.

If we would like to convert entity annotations, it means raising our minimum supported version to 11.1. This may be acceptable if we consider it's a new module, and the majority of sites using CRM may be Drupal CMS sites, which will be D11 based from the outset.

One other thing I noticed. When markdown_easy.settings is set, the status page still warned about the missing filters.

Is this the correct behaviour?

I set the skip filter enforcement with:

ddev drush config:set markdown_easy.settings skip_filter_enforcement 1

I then tested that I could disable the other filters without issues.

Next, I set skip input stripping with:

ddev drush config:set markdown_easy.settings skip_html_input_stripping 1

I tested that tags added in the node with the filter applied were allowed to pass through.

These both behaved as expected.

Setting back to 0 prevented tags in source from passing through, and forced the required filters to be enabled again.

Does this need something in README.md covering the new settings (and how to set them without a UI).

Also, there's a new install file for markdown_easy.settings.yml to set up the new settings when first installing the module, but these will not be created for existing installs. I don't think this will be an issue, as the settings will default, but should there be an update hook to add them to keep things tidier?

That all sounds pretty reasonable.

Keeping the 'html_input' => 'strip' sounds fine to me. If anyone really wants to live dangerously, they can override with the hook.

I think what you propose for the MVP makes sense. We could possibly add enhancements in follow up issues.

I agree this only makes sense in 1.x. I've reviewed the change and it makes it a lot clearer for the end user.

Moving to RTBC.

Okay, so we might have a "skip filter enforcement" mode, which does not require the HTML filter to be enabled, and does not warn about missing tags in the filter.

There are several options for the actual check that spring to mind:

• List the expected tags for the Markdown "flavor" on the form.
  • Ideally the recommended list would change when you select a new flavour, without requiring a page refresh.
  • A nice to have would be some JS that checked the list of recommended tags as you input and lists missing ones.
  • Another feature to consider would be a button labelled "Apply recommended tags". Clicking it would replace the current HTML filter list with the entire list of recommended filters.
• On form save, if all recommended tags for the flavor are not in the filter list, display a warning listing missing tags.
• On the site status page, if there are missing tags, display a warning listing them.

One other thing to consider is allowing the use of Markdown without 'html_input' => 'strip', so that users would be allowed to input HTML, but the HTML filter would restrict the tags they could use. A hidden setting like above would make sense for this, but I could see two approaches both being useful:

• Allowing HTML in the input, but relying on HTML filter to prevent any undesired tags.
• Trusting that HTML is removed by the Markdown library, so we don't need to filter the generated HTML.

I've added the config to generate the correct HTML to use a CSS class. Also added a test case to verify.

Still need to add the library and link to the output HTML.

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

I hadn't realised that the HTML stripping was also in the CommonMark library. I had assumed the part of the reason for the HTML filter was the catch any HTML in the input.

If we retain this going forward, we should probably better document that HTML isn't allowed in the input, as the Markdown spec does offer the option of including HTML tags in a number of cases.

I'm happy to work on this, but I think we need a bit more clarity on the correct direction before committing development time to it.

jdleonard → credited lostcarpark → .

Those are good ideas. Would be nice to have a "side by side" view that automatically updated periodically as you type.

I see your point about using AJAX so the preview is using the same processor as the final output, though I do like the idea of at least basic formatting being applied in the editor.

jdleonard → credited lostcarpark → .

Updated to remove the old version of the puzzle piece icon.

I can't see how having the default icon a lighter color conveys any useful information to anyone except the maintainer of the module providing the field. If anything, it might make people think the field type is unavailable.

However, if anyone feels strongly that it should be a lighter shade, I can certainly find a color that gives sufficient contrast.

It's also worth noting that the Gin theme seems to override the default color for all icons, so in Gin the placeholder is a dark color by default, and a light color in dark mode.

I've searched the core codebase, and there are no other references to the puzzle_piece_placeholder.svg file, so I agree it makes sense to remove it.

I have made the puzzle piece the same colour as the other core icons. The new one is in the core/misc/icons/55565b directory.

The original version remains in the core/misc/icons/cacbd2 directory. I'm not sure if it should be removed.

Moving to needs review.

I'm working on this.

lostcarpark → created an issue.

Note that this change is against the 1.0.x branch. I feel this is correct as it's a bugfix.

However, once RTBC, it should be cherry-picked into a second MR against the 2.0.x branch.

I wrote a new test class, InstallTest to create a Markdown input format, then install the Markdown Easy module. I verified this failed, then moved filter.format.markdown.yml into the config/optional directory, and verified the test passed.

Pushed as MR !17.

Ready for review.

lostcarpark → changed the visibility of the branch 3496182-markdown-easy to hidden.

lostcarpark → created an issue.

Moving forward

None of the problems mentioned above directly relate to the original request in this issue, which was to add support for footnotes. We've already had a fair bit of scope creep on this issue!

Also, the change in this issue has already been merged, and rebased with 📌 Remove/lesson dependency on "Convert line breaks into HTML" Active . From a selfish point of view, I worry adding more to this issue will result in a very complex rebase.

I propose moving this issue back to Fixed, and creating follow on issues for the problems above. It seems to me that the 3 problems (changing flavor, tables, and improving test coverage) should each have their own issue (the first two should, of course, have their own test cover, and the third should deal with improving coverage of already supported Markdown).

Test Coverage

I agree this needs improving, though it's funny you mention strikethrough, as that is covered by the current tests (and seems to be supported in the CommonMark "standard Markdown" flavor, even though it's a non-standard feature).

I would like to see test cases for specific Markdown covered by different flavors, to verify it gets processed only by the respective flavor(s).

I already added tests for line breaks in 📌 Remove/lesson dependency on "Convert line breaks into HTML" Active , and I would like to add additional tests for Markdown syntax.

Tables

I definitely agree we shouldn't use deprecated attributes!

I don't love inline styles, as they create the potential for people to commit all kinds of crimes against CSS, though I think modern browsers protect against most forms of malicious CSS injection threat.

I presume we could instead use something like:

 $config = [
    'table' => [
        'alignment_attributes' => [
            'left' => ['class' => 'markdown-align-left'],
            'center' => ['class' => 'markdown-align-center'],
            'right' => ['class' => 'markdown-align-right'],
        ],
    ],
];

And add a library with CSS to be included with the filter.

We'd then be adding the allowed tags: <table> <thead> <tbody> <tr> <th class> <td class>.

The downside is, of course, that we'd be adding the library, and including CSS whether the Markdown contained tables or not.

Oops, I accidentally changed the status... changing back.

Some very good points here.

Changing flavor

I like the idea of adding allowed tags when you change the Markdown "flavor" (option 2)... but if we change the allowed tags on form submission, we would potentially be changing configuration (of the "Allowed tags" filter) the site admin deliberately selected, causing unexpected results.

So I'm thinking if we go that route, we should add a library and run some JS when the Markdown flavor changes to add appropriate tags.

Should we also take away tags if the user changes to a more restrictive flavor?

I also like the idea of warning if tags for the enabled Markdown flavor are not in the allowed tags list (option 1). I do this for <p> and <br> in 📌 Remove/lesson dependency on "Convert line breaks into HTML" Active .

Option 3 is already supported... sort of. You need to use a hook to allow it, as can be seen in markdown_easy_test_module. I would not be opposed to reducing the current error to a warning to enable this more easily, but I guess Mike has to decide on that.

I kinda like option 4. It would allow us to limit the tags in the source but allow the library to add what it needs. Again, Mike's call on whether he trusts the library.

Rebased against 2.0.x branch, and fixed conflicts.

• I added a test, testParagraphs() that line breaks are handled correctly.
• I updated the checks of Markdown Easy filters in Status report, and updated the test of it using xpath, as the CSS based checks seemed to always be passing, even when they shouldn't.
• I updated the "tips" list on the config form, which was including the list HTML in the translatable string. It's now using an item_list render array, with separate translatable strings.
• Finally, I've updated the README, and hope I've adequately covered the changes.

Some concerns:

• We now have 6 bullets in the "tips" on the filter page. Is that too much, and is there scope for rationalizing?
• The README could do with some careful review to make sure my additions are correct, and that I haven't missed anything.

Thanks for the feedback.

I'll make sure I update the README.

I also want to improve test coverage (there's one test that's passing that I think should fail with this change, so I'm wondering if it was actually working as expected).

This change didn't require much, other than disabling the requirement for the "Convert line breaks" filter.

Once disabled, all that's needed is to add <p> and <br> to the list of allowed tags.

CommonMark then handles line breaks correctly.

I would like to add some test cases around correct formatting of line breaks, then should be ready for review.

Yes, the raw HTML output part should be resolved now.

It's also worth noting that the current implementation does not correctly handle line breaks in accordance with the Markdown spec.

Currently line breaks within a paragraph always get converted to a <br> tag. Markdown is intended to allow paragraphs to be formatted with line breaks in the source document, to allow easy readability while composing. To insert a line break, a line should either be ended with 2 or more spaces, or a HTML <br> tag.

I think removing the reliance on the "Convert line breaks" filter would allow CommonMark to handle line breaks correctly according to the Markdown spec, though I haven't yet tested this.

I have removed the ->render() from the error messages, and stripped out HTML from the expected results. Tests are now all passing.

After a lot of frustration and tinkering the solution turned out to be annoyingly straightforward - just strip the HTML out of the assert statements!

lostcarpark → created an issue.

jdleonard → credited lostcarpark → .

Added mentor ticket (Vienna) application details.

Renumbered to move Nara next after Vienna, followed by upcoming camps, and "meta" last.

Merged successfully. Thank you @lavanyatalwar and @nidhi27 for your work on this.
Moving to Fixed.

I've confirmed the needed changes have been made.
I've tested the update hook in a local environment, installed with the 1.0.0-alpha4 release, than checking out this branch and running the update script with Drush.
I have verified that the token was added to the email body as expected.
Moving back to RTBC, ready for merge.

A couple of small points. It's helpful to use a type hint comment so that IDEs know the class to use and can offer code completion and warn about missing functions.

Also, where a specific getter is available, we should use it rather than the generic get().

Fix those and we should be ready to merge.

Added mentor ticket question to Nara section.

lostcarpark → created an issue.

Merged into 1.1 and 1.2 branches, moving to fixed.

Went through with @ultimike, and he suggested removing <front> from the access check, as we're no longer using it as a route.

Thanks for the review, @ant1.

Going ahead with a merge.

Merged successfully. A nice improvement to test coverage.

Thanks for working on this.

I've reviewed and run the tests locally, and this change looks perfect to me.

Moving to RTBC.

jdleonard → credited lostcarpark → .

Yes there is "clone issue" in sidebar! Never noticed that before. Maybe when in the final documentation we could have a screenshot.

Added 1.5 about Volunteer app.

@mradcliffe - you forgot the most important step - updating the icebreaker question!

More seriously, I don't see the clone function, and Dreditor project has been abandoned 5 years ago, and I'm not sure there's an install for it still available.

lostcarpark → created an issue. See original summary → .

That looks perfect now, and all tests are passing.

Moving to RTBC.

@lavanyatalwar, thank you for working on this issue, and thanks for your patience with my nit-picking.

Merge completed, moving to fixed.

That looks perfect now, and all tests are passing.

Moving to RTBC.

Sorry to be picky, but I think the description could be a tiny bit clearer.

Also, sorry for not including in the original request, but I think we should have a test case for this. This can be added in VerifyEmailSetupTest.php, in the testVerifyEmailConfig function, just before submitting the form. Add the following assertion:

$this->assertSession()->elementExists('xpath', '//div[@id="edit-email-body--description"][contains(text(), "Email message to be sent to user. Place token [verify-email:url] to insert the login link")]');

I promise this is the last change!

Thanks for working on this. Some change to the text is needed. In particular, it needs to reference the available token.

Adding contributors.

Thank you for working on this. Another little step forward.

lostcarpark → created an issue.

Perfect! Moving to RTBC.

Thanks for working on this.

There's just one minor issue, the token should use a hyphen rather than an underscore.

We should then be ready to merge.

Merged and released version 1.2.0.

PHPStan error is due to Project Browser updates for Package Manager being addressed in 📌 Conflict with Project Browser <2.1.0 and Automatic Updates <4.0.0 Active , so not a concern for this module.

I've added a test of pressing the "Clear storage" menu, and verifying the correct response.

I'm moving this to RTBC.

I've completed the addition of a "Tools->Project Browser->Clear storage" option.

Had some difficulty getting tests to run for Next Minor in GitLab CI, but thanks to @cmlara on Slack, I figured out the correct versions to specify in composer.json.

Still getting a PHPStan error for next minor.

lostcarpark → created an issue.

One more small change. In the NavigationExtraToolsAccessTest test, I had hardcoded the strings the test asserts against, which are used multiple times. I have moved these into constants, which reduces duplication, and makes the code more readable.

Another small update.

I figured out the controller core "parent" menus use, which displays the child items as a list.

Using the same controller for the "Tools", "Clear individual cache", and "Development" menus that if a user ends up on one of those pages, they will see available options in a list (or a message letting them know they don't have access to any menu items).

Although this should not happen in normal circumstances, it means that if the user does land on the Tools menu page, the menu will be displayed consistently with other menu pages.

This also means the individual controllers for those pages are no longer required.

Would be very grateful if anyone has time to test.

Suggested test steps:

1. Install current stable version (1.1.x branch) of Navigation Extra Tools.
2. Check that admin user can see Tools menu on sidebar, with cache clearing options under it.
3. Create user with access to Navigation, but no admin functions, and verify that Tools menu displays, and empty "Flush individual cache" submenu shown, but no other options visible..
4. Install this issue fork, and either uninstall/reinstall, or clear caches.
5. Check that Tools no longer shows for user with only access to Navigation.
6. Grant user access to "Navigation Extra Tools Clear Cache", and verify Tools menu visible (and cache clearing options under it).
7. For bonus points, take away their cache clearing privilege, but then install the Devel module, and give user "Access Devel Information" privilege. Check they can see Tools again, and only option under it is "Development".

Obviously will give contribution credit for testers.