🇮🇪Ireland @lostcarpark

Confirmed all tests are passing.

Moving to Needs Review.

Edit title to remove question

Scheduled Publish 4.1.0 has been released, and should work with Drupal 9, though it hasn't been explicitly tested against it. If there are any security or bugfixes against the 4.1 branch, they will continue to support D9.

However, new features will be developed against the 4.2 branch. In particular, 📌 Convert procedural hooks to object oriented hooks Active is in progress to convert to object oriented hooks. As this feature requires autowired services, it is not possible to implement keeping support for Drupal 9. Therefore, this change is to remove D9 support from the 4.2 branch.

Merge complete.

I appreciate the additional test coverage.

Thank you for your work on this.

I've carried out some manual testing, and merged in previous changes.

Happy to merge in.

I created a test module containing a new plugin with an ID of testflavor, and a label of "Markdown Easy Test Flavor".

I've added a test that tries creating a text format using the plugin, checking that the new flavor exists in the drop-down, and that the correct tags are warned about.

The expected tags are the "standard" list plus <article>.

This looks almost perfect.

Upgrade Status is giving a warning about core_version_requirement in the test theme. I know that line can be omitted in test modules, so I suspect it could be removed from the test theme too. I would try deleting the line and see if the tests pass.

I realised that findMissingTags in MarkdownEasyUtility hard coded the list of tags for each flavor, and also raised an exception if an unknown flavor was used.

So it seemed to make sense to add the expected tags to the flavor attributes.

I though we should keep the "tag sets" so we don't have to define the full list of tags in every plugin, which would require a lot of repetition.

So I've added two new plugin attributes:

• tagSets is an array of tag sets, with the allowed values "standard", "github", and "markdownsmorgasbord".
• additionalTags is an array of individual tags, with the tag name as the array key, and an array of attributes as the value.

This required the MarkdownEasyPluginManager to be injected into the MarkdownEasyUtility service.

This in turn required the MarkdownUtilityTest unit test to be updated with a mocked plugin manager service.

I'm tempted to add a test class that implements a minimal plugin, which should help catch people accidentally adding hard coded stuff in future.

This should be a fairly easy change for someone aiming to build up their contribution experience.

Here's a brief summary of what's needed:

1. Create a Hook directory under /src.
2. Create a new PHP class in the /src/Hook directory. As all the current hooks relate to altering forms, I'd suggest calling the class FormAlterHooks.
3. Create a __construct() constructor to set up dependency injections. It should be able to accept the services: config.factory, string_translation, markdown_easy.utility, messenger.
4. Copy the hook functions from markdown_easy.module into the hook class. As a general convention, the "markdown_easy" prefix should be dropped from the prefixesshould be dropped, and the function names should be converted from snake case to camel case.
5. Add the #[Hook] attributes to the functions. For example the current markdown_easy_form_filter_format_add_form_alter function might become formFilterFormatAddFormAlter, and its attribute should be #[Hook('form_filter_format_add_form_alter')].
6. Functions starting with a prefix are utility functions, and can be converted to private methods of the class.
7. Any \Drupal calls should be replaced with the dependencies injected in the constructor.
8. Add an entry to markdown_easy.services.yml to give the hook class a service name, and specifyautowire: true.
9. In the hook functions in the module file replace the function code with a call to \Drupal::service() to call the hook service, and call the equivalent object oriented hook.
10. Autowired services require Drupal 10.1 or higher, so older versions should be removed from the .info.yml and composer.json files.

PHPStan was reporting Unsafe usage of new static.

Used example from this page → to update phpstan.neon.

I think the use of plugins makes the code much cleaner and maintainable.

However, I'd like to propose a few small changes.

First, the Flavor dropdown was populated with:

      '#options' => [
        'standard' => $this->t('Standard Markdown'),
        'github' => $this->t('GitHub-flavored Markdown'),
        'markdownsmorgasbord' => $this->t('Markdown Smörgåsbord'),
      ],

This negates one of the big advantages of plugins, which allows new plugins to be added without needing to modify this module.

Instead, we should populate using $this->markdownEasyPluginManager->getDefinitions(), which allows new plugins to be included in the list without any code change. This would allow either contrib or custom modules to provide their own additional markdown flavors.

Also, the Markdown Smörgåsbord had an ID of markdown_smorgasbord, but the config setting is markdownsmorgasbord, which was requiring a match() to map them correctly. That's the only place an underscore is used in the flavor, and changing the ID to markdownsmorgasbord allows that to be simplified, and removes the need for the match().

I attended!

lostcarpark → created an issue.

lostcarpark → created an issue.

The hooks look good, but PHPStan is giving a warning for previous major, because the hook attributes are not known in D10.

I have added a before script to .gitlab-ci.yml to delete the attribute lines from the source file before PHPStan runs for previous major. This will only take effect if the Drupal major version is 10.

Currently the info.yml file contains:

core_version_requirement: ^9.5 || ^10.2 || ^11

When 4.0 released, D11 hadn't yet released, so 9.5 was still supported.

Merged successfully.

Thanks for working on this.

Looks perfect, and all tests are passing.

Moving to RTBC.

I have reviewed the test change, and it all looks good.

I ran the "test-only changes", which rolls back all changes that aren't to tests, and runs the changed tests against the unchanged module code.

This failed as expected.

Moving to RTBC.

lostcarpark → created an issue.

lostcarpark → created an issue.

lostcarpark → created an issue.

This shouldn't cause problems, though I've messed up composer changes before...

Gitlab seems to be having some issues at the moment. Will check the pipeline and rerun if necessary.

lostcarpark → created an issue.

That sounds like a good strategy to me.

• Extend the test.
• Verify it fails without this change.
• Verify it passes with the change.

The new tests look pretty good!

I will do some local testing, then merge.

Hoping to get a release out this weekend.

I missed a PHPCS error about an extra line break at the end of the libraries file. Fixed now.

Thanks for your help, @sandeep_k.

Thank you for reviewing. Merging.

Merged successfully. Thank you for working on this.

StyleLint was failing, but was not related to this change, and already fixed in another issue. I have rebased this issue, and StyleLint and all tests now pass.

The new test looks good. I will give the change a final check over and merge soon.

Thanks for your work on this.

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

Merged and all tests pass, so moving to fixed.

I need to carry out a final test, but I think this issue is resolved by 🐛 Broken URL for editing a scheduled item Active , so it should be good to close.

Thanks @vitaliyb98 for your work on this!

Change merged successfully.

I've done some testing and verified tests are passing. Ready to merge.

Thanks for working on this.

There are several issues covering multilingual support, and I'd really like to merge them. However, I feel that we need some test coverage. I worry that someone (probably me) will make a future change and forget to test multilingual features and break something.

So I think it's important to have basic test coverage.

If you could outline the steps to test from a fresh Drupal install, that would be a great help.

Thank you for working on this. Patch files are deprecated, so patch will need to be converted to a merge request (or the existing MR !24 updated).

If someone has time before I get to it, please go ahead.

I feel this should also have some test coverage, as a lot of developers working of future features may not test on multilingual, so there should be at least a basic test of multilingual functionality, or it's likely some future feature release will break it.

Seems a nice straightforward fix, and works for me in test.

Sorry I haven't looked at this issue. It would be great to try to move it forwards. There are some outstanding questions in #9, that it would be great to get opinions on.

Merged successfully.

I have checked the change and it looks perfect.

There is a failure of phpstan (previous major), but that is not related to this change, and I've opened 📌 Fix PHPStan for previous major Active to resolve.

Will merge now.

lostcarpark → created an issue.

I attended.

That would be great. A good start would be to outline the steps a Kernel test needs to follow. It might give someone a head start before you get to it.

Looks good!

Any thoughts on how we could add test coverage for this? I'd be happy enough to merge as is, and hopefully add test coverage later, but if we could easily cover it off, it might be worth putting some effort in.

Thank you! The problem with pushing changes when I'm tired! I did a git status then pushed the files that had changed, but forgot the new files. Hopefully fixed now.

Also present.

Merged into 4.x branch.

Moving to fixed.

Icon added as shown:

Ready for review.

lostcarpark → created an issue.

Thanks for this. Omitting the version makes sense. All tests pass.

Rebased for latest 4.0.x changes. Tests are passing.

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

I have rebased to include 🐛 Unable to update a scheduled record Active , and verified tests are now passing.

Ideally someone else should move to RTBC since I developed originally.

I have merged the latest change. I would love to add test cases to fully cover this, but don't want to delay other pending changes.

There was a PHPCS issue reported on previous test run, but it went away when I retested, but a Stylelint issue appeared instead, so I tweaked some selectors to fix.

I have run tests locally and carried out a manual test. Ready to merge.

Tests are now passing on 1.4 branch.

Aim to continue development on this soon.

Merged and moving to Fixed.

As this is adding a step top the pipeline, and the pipeline is passing, I'm happy to merge this.

It would make sense to use <!--break--> to end the text if it comes within the smart trim limit.

For example, if Smart Trim was set with a limit of 100 characters, and a <!--break--> was present at character 50, then Smart Trim would return up to the <!--break-->.

However, if the <!--break--> came at character 150, then Smart Trim's normal rules would apply, and only the first 100 characters (adjusted for word breaks) would be returned.

As an extra module is required to support the <!--break--> with CKEditor, I don't think this is widely used, I wonder should there be a config option added to enable support of <!--break-->? If selected, text would be trimmed at <!--break-->, if it appeared within the trim length. If not enabled, <!--break--> would be treated as a comment.

lostcarpark → created an issue.

lostcarpark → created an issue.

lostcarpark → created an issue.

Okay, I didn't notice the branch I was pushing to was already merged.

New branch and MR opened. Hopefully that's somewhat right now.

I have updated the PHPStan page with a section on preventing object oriented hooks from causing errors on phpstan (previous major) due to unknown attributes.

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

Rebased for 1.4. Needs work to fix tests.

Made the change and all tests are passing.

lostcarpark → created an issue.

Merged and released in 1.3.1.

Thanks for the issue and the patch.

Tests pass!

Merging and releasing 1.3.1 as current version could block installs.

Good catch! Converting patch to MR.

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

I believe I have fixed the annotation PHPStan issue in 📌 Fix PHPStan for previous major Active . If that fix is valid, I suggest merging that first, then rebasing this one.

I tried adding @phpstan-ignore-next-line lines, which worked for me in another module, but these were giving "no error to ignore" errors when testing against current versions.

I'm not very happy with the approach of adding ignore statements, since they could prevent genuine issues from being missed.

So I thought it would be good if it could be added for the previous major, which is the only time it's needed. Adding a before_script to insert the ignore lines.

However, after getting the tests to pass, I realised that deleting the attribute lines would achieve the same thing, but makes the script commands much simpler. So the updated .gitlab-ci.yml file includes a before script that runs for "phpstan (previous major)" only, that deletes the attribute lines.

I hoped to make this issue only about the attributes, but phpstan (next major) is required to pass, so I copied mike's change from ✨ Allow the possibility to specify which tags to be stripped when Strip HTML checkbox is ticked. Needs work to fix the deprecation issue.

lostcarpark → created an issue.

Issuing credit and moving to fixed.

I attended.

I attended.

I don't think there has been any work on Advanced Forum for many years.

The Forum module → is now in the contrib space, and does seem to be receiving some attention. I think a better goal might be to make Forum more like Advanced Forum than revamp this project.

jdleonard → credited lostcarpark → .