🇮🇪Ireland @lostcarpark

Also encountering on Navigation Extra Tools module in ✨ Use Icon Api for adding icons in Navigation items Active . Got tests to run by adding "phpunit/phpunit": "<=11.5.28" to require-dev section of composer.json, but don't want to merge that.

I've been fixing up the tests as the classes have changed due to the new icon API way of doing things.

Unfortunately there seems to be a general issue with PHPUnit 11.5.29, affecting many projects (see 🐛 PHPUnit job hangs as of update to PHPUnit 11.5.29 Active ), causing PHPUnit to hang and eventually time out. I have temporarily set the maximum version to 11.5.28 in Dev dependencies, which allows tests to pass. I don't want to merge this, so will need to wait for a fix for that issue.

Also seeing an issue with PHPStan on previous major due to hook attributes. I'm sure this was passing before so looking into the cause.

Setting to needs work for the moment, till I figure out the PHPStan issue, but also blocked by the PHPUnit issue.

And the project page changes look good to me.

Yes. Moving to RTBC on the assumption of that addition.

This all looks solid to me. We might also add 📌 Doublecheck settings being merged Active , which is all code removal, so I don't think needs any documentation or Readme change, but might need a brief note in the release notes, something like:

• Removed merge of saved settings with default when loading form, also removed blank "tips" from schema, and removed use of "Convert line breaks" filter from some tests.

Added Drush command to update advanced configuration settings.

I've removed "tips" from the schema and default Markdown text format. I don't think it's needed, and it is never updated by the settings form, so it will only ever be an empty string.

I've also removed the "Convert line breaks" filter from some tests where it's not relevant.

Also removed a check that the "Convert line breaks" error is not displayed. The error message is no longer in the module, so will never be displayed.

Moving back to needs review.

I have reviewed, and I agree that merging with the default settings isn't doing anything useful.

I also agree returning an empty string for tips in defaultSettings isn't necessary.

Moving to RTBC.

I think this is a brilliant idea.

However there are a number of code quality issues being reported by PHPCS and PHPStan, so those would need to be fixed.

Unfortunately the bigger issue is that I don't think new development is happening on the 1.x branch, and a number of new features have been implemented on the 2.x branch.

I would recommend:

• Rebase against 2.x, and fix any conflicts.
• Add a plugin for the "Markdown Smörgåsbord" flavor. Could this be extended from the GitHub flavor, since its a superset of its features?
• Consider additional cases to explicitly test the plugin system.

Setting to "needs work".

Hi, just learned about these and would love to attend on the 4th!

The project page lists time as 5pm UTC and also 5pm Irish/GMT (thinks for giving Ireland a mention).

They are the same in the winter, but currently 17:00 UTC is 6pm IST (also BST), so just wanted to check which it is.

I've carried out some manual testing, and the module is behaving as expected, and allows "Convert line breaks" filter to be disabled, as well as warning if it's enabled.

Tests are passing so happy to move to RTBC.

jdleonard → credited lostcarpark → .

This looks great! I'm all for anything to make the module more generic. I've flagged a couple of minor PHPCS issues, and there are a couple of tests that need updating. I will review and try to get this and other issues merged over the next few days.

Sorry, commented on the wrong issue.

Looks good to me.

I have tested to verify that the item list is correctly formatting as an unordered list, and that a tip for "Standard markdown" has been added.

I have tested that when skip_filter_enforcement is enabled, the tips are not shown.

This looks good to me!

That should be fairly easy to do. I was thinking of adding a config page with an option for the icon position.

lostcarpark → created an issue.

lostcarpark → created an issue.

Sorry for the delay. I have carried out manual testing and I believe this is working as expected. I think the tests also cover the functionality quite well, and all pass.

I do have a slight reservation that missing tags on the HTML filter are only flagged when the filter is saved, so you have to edit again to correct. This is as designed, but perhaps we could look at improving the user experience in a follow-on issue.

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 → .