🇮🇪Ireland @lostcarpark

jdleonard → credited lostcarpark → .

jdleonard → credited lostcarpark → .

This looks a useful addition.

However, the regex replacement is always applied if the Shortcode module is enabled, regardless of which text format is being used. If shortcodes are only enabled on one text format, it shouldn't affect other formats.

Also, it seems to treat anything in square brackets as a short code, resulting in the short code not being trimmed. I'm not very familiar with the short code module, but I think it will ignore anything in square brackets that's not defined as a short code. I think a long piece of text in square brackets would therefore get left in the text and not trimmed, which is probably not the intended effect.

I'm setting back to "Needs work", as I think this needs some more thought, and maybe some explicit test cases, but if I've misunderstood, please explain in a comment.

lostcarpark → created an issue.

I attended.

Merged and will release version 1.3.0 shortly.

Thanks @rmpereira for the initial idea and MR, and thanks to @bhogue and @ultimike for reviewing.

Although there is a minor issue that changing the options on the Devel Toolbar page requires a cache clear to take effect, I don't want to hold up a release any longer, so I will merge and create a follow up issue.

lostcarpark → created an issue.

Also, if implemented, it probably should be power_user_mode for consistency with the other settings.

A little care is needed, because this would be creating 2 ways of achieving the same thing. This could cause confusion if someone is trying to set skip_html_input_stripping to false, and wondering why it's having no effect because they didn't realise power-user-mode was true.

I have added an update hook to call the Devel Menu service to update the initial development menu contents to match the hard coded entries from earlier versions.

Moving to Needs Review.

jdleonard → credited lostcarpark → .

I was there.

Added checks for Config Editor menu, which will only be present if extra items enabled by Navigation Extra Tools.

Main thing left is to add an update hook, and associated test, and then will be ready to go back to needs review.

I have added a test that uninstalls navigation extra tools, then reinstalls it, and then does the same for the devel module.

This tests that devel options get enabled whichever order the modules get enabled. I want to add a check for specific menu items that are enabled during install.

jdleonard → credited lostcarpark → .

Added service to enable the previously included Devel menu items.

Called from hook_install and hook_modules_installed, to catch devel being installed after navigation_extra_tools.

Fixed up tests.

Would like to add a test that covers installing navigation_extra_tools before devel, and vice versa.

Need to add an update hook to enable options when upgrading.

Would like to generate a fixture and add a test of the update.

Ah! I found the route in the Devel module, and realised it has a dependency on the Toolbar module. I normally disable Toolbar when I enable Navigation, hence I was not seeing it.

I think it would be worth opening an issue in the Devel module to get this dependency removed, or to make dependent on Toolbar OR Navigation.

I'm still feeling the best solution is to enable a larger set of options during install.

The problem I see with this change is that the default development menu options are very much reduced from what 1.2 provides.

Here's the current list in 1.2:

Here it is after this change:

I am not finding the "Devel Toolbar Settings" page shown in #5. Is it provided by another module?

While being able to customise the options on the menu is useful, I'm sure there are other users like me who don't know this is possible.

I can think of several solutions:

1. Add a recommendation to the project page/README to install the module providing the Devel Toolbar settings page. I think this would be useful to do, but I would not favour as the only solution.
2. In ToolsMenuDeriver, detect if module providing Devel Toolbar settings is enabled, and if not, add the options that 1.2 includes that the Devel module does not add by default.
3. During Install, enable the missing options in the same way as the Devel Toolbar settings page. An update hook would also be needed for those upgrading from earlier versions.

My preference would be option 3, as it's fairly seamless for existing users of the module.

Tests need fixing up.

Would be nice to get fixed up and included in a new release.

Thanks for working on this, and thanks everyone for testing.

Merged into 1.3 branch. Will try to get a 1.3 release out later this week, but seeing what else can be included.

Rebased for 1.3.x.

Moving to 1.3.x.

Moving to 1.3.x.

Change target version to 1.3.

The need to add @phpstan-ignore-next-line so hook attributes won't trigger PHPStan warnings seems a bit ugly, and has potential to cause genuine errors to be missed.

If Drupal 10.6 were to define the attributes, even if they did nothing, it would prevent the PHPStan warning for previous major, and allow these lines to be removed.

I don't know enough about PHP attributes to know if this would be possible, but if PHPStan for previous major could warn if a [Hook] was implemented without a [LegacyHook] counterpart, that would be really helpful.

PHPStan has been added to GitLabCI for previous major. This is causing both [Hook] and [LegacyHook] to be flagged as unknown attributes.

I tried like this:

/**
 * Comment about the hook.
 */
// @phpstan-ignore-next-line
#[LegacyHook]

But it resulted in a warning about an empty line.

What actually worked was:

/**
 * Amazing hook comment.
 *
 * @phpstan-ignore-next-line
 */
#[LegacyHook]

I needed to add this to both the .module file and the Hook class.

I have a concern that this will cause actual errors in the hook attributes to be missed, particularly when attributes for specific hooks are implemented in future.

PHPUnit now locked to 11.5.28 for GitLabCI.

Also, thanks to @jurgenhaas for showing me how to correct the PHPStan error. I added @phpstan-ignore-next-line to the comment above the attribute.

Moving back to Needs Review. Would be greatful if someone could review my changes.

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.