🇮🇪Ireland @lostcarpark

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.

After reviewing with @ultimike, I have made some changes:

• I've made both the main tools menu and the Devel menu call the same access function, and use the route name to select which check gets used.
• I was fetching the whole Admin menu, and then selecting the Tools submenu from within it. I've figured out I can use setRoute in the MenuTreeParameters to select the specific submenu I want. I've also told it to only select one level of the menu.
• I was traversing the whole Tools menu tree to find all the items the user has access to. I've realised that once I find one item the user has access to, I know they can see the Tools menu, so I don't need to look any further, so I can return from checkTreeAccess as soon as an item is found.
• I was recursively checking the whole tree under Tools menu, but I've realised any submenus under Tools will have their own permission, so I only need to check for access to items on the first level below Tools. That allows me to drop the recursion, so makes the check simpler.

Change merged successfully.

Thanks for your work on this!

I have reviewed the test, and it looks perfect to me. It confirms that the correct result is being returned from the token replacement, and the custom token is applied.

The tests are passing again, and I have run locally to confirm.

Moving to RTBC.

Merged. Moving to fixed. Thanks for working this, @nidhi27.

I've reviewed this change and am happy to merge. Hopefully we are moving closer to tests passing.

Moving to RTBC.

Accidentally specified the class name as "TokenHooks" when it should have been "TokensHooks". Thanks for catching this, @nidhi27.

Tests currently failing because of empty test class, but will be fixed soon.

Merged, though we have test failures. Will be fixed by 📌 Add test of token replacement Active .

Thank you for working on this.

This looks great to me.

The tests are producing a warning because there are no tests in the class, which is getting marked as a failure in PHPUnit. However, this should be resolved in the next issue.

Moving to RTBC.

Thank you for your work on this issue. Now merged.

I have reviewed and carried out some testing, and I can confirm this behaves as expected.

Moving to RTBC.

Thanks for your work on this.

Looks perfect. Merging.

lostcarpark → created an issue.

lostcarpark → created an issue.

lostcarpark → created an issue.

lostcarpark → created an issue.

Thanks for your work on this. Moving to fixed.

lostcarpark → created an issue.

I've reviewed and carried out some very basic testing, and I believe this change is ready to merge.

Moving to RTBC.