🇧🇪Belgium @Frederikvho

I know this an older issue, but I ran into this recently and wanted to document my solution in case anyone else runs into this.

This is what solved it for me: I added the migration to my custom module's /migrations folder prior to enabling the module. I am guessing core's migration discovery discovers the deriver this way.
Because now the config gets created, without deleting the deriver parameter.

This was also good post for me: https://www.webomelette.com/dynamic-migrations-using-templates-drupal-8

Interesting, I didn't get any other phpcs errors with my last commit. And sorting use statements was what I had fixed on top of applying patch from #3.
Now with the latest addition in #8 I do get phpcs errors, about sorting use statements, again.

What version of phpcs and drupal/coder are you running? I just verified, and I am running the latest version.
phpcs: PHP_CodeSniffer version 3.10.3 (stable) by Squiz and PHPCSStandards
drupal/coder: 8.3.25

Converted previous patch to MR and sorted the imports.

Thanks for testing.
This is now in the latest 2.0.0-alpha3 release.

frederikvho → created an issue.

frederikvho → created an issue. See original summary → .

Thanks for the patch.
This will be in 2.0.0-alpha2, D9 compatible.

Thank you for the approval and help avpaderno.

Yes I hope to get it to D10 soon, rework the core functionalities of the project to get a clearer view, some of the classes need modernizing.

When it becomes more stable security optin is definitely next on the list.

Drupal 9 version is released on ' 2.0.0-alpha1 ' , trying to get a more stable release soon on 2.x and D10.

Hi,

We are interested in using this module for one of our client's sites, but as it's a Drupal 10 site we would need and like to convert it to be D10 compatible.

As this issue has not received feedback since 2021, I would like to offer to maintain the issue to be able to do the needed Drupal updates, get stable releases out and hopefully opt in for security coverage at one point.

Kind regards,
Frederik

The info.yml file needs to change:
remove - core
and add - core_version_requirement instead.

This issue needs to be merged as D10 and 11 are there.

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

I have sent @andypost an e-mail about this issue using his contact form.

frederikvho → created an issue.

+1 RTBC

This could do with a new release

This code improvement works for me, +1 RTBC

As the D8+ compatible releases have been out for a while now I think this issue can be closed.

I cannot reproduce it running D10.3 and PHP8.2.

I don't see duplicate variable declaration in the PHP class on the dev branch either.

Maybe you had a clash with a patch or this commit: https://git.drupalcode.org/project/domain_entity/-/commit/0b0b254c5bdd5b...

Don't know if there are any other typos but thought I would correct these already.

frederikvho → created an issue.

frederikvho → created an issue.

The issue fork is currently mixed up, some classes got duplicated code due to merge conflicts between the major branches 3.x and 8.x-1.x.

Here is an updated patch for 8.x-1.x.

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

I tested this and it works for me +1

I just wanted to add that I received a logical error when trying to update this module, because the removed dependency is still installed in the active configuration upon deploy. This causes drush updb to fail. This is normal Drupal deploy behavior and cannot be prevented afaik.

 [error]   (Currently using Missing or invalid module The following module is marked as installed in the core.extension
configuration, but it is missing:
 * dynamic_entity_reference

I solved this by doing two deploys:
First composer require the dependency that was removed in the latest update.
The rest of the flow can stay the same, so:
dependency should be removed from core.extension.yml file
Then deploy.
Only difference is Drush updb won't fail this time around. Because you will have put the dependency back in place separately.

For the next deploy you can remove the required dependency, once and for all.

Both this updb error and my solution seem logic to me, keeping the Drupal deploy strategy in mind, so I didn't see the need to create a new ticket. I just wanted to put this out there in case other developers struggle with.

Hello,

Why was this issue closed? The merge request is still open.

Also, is this contrib module still needed as core made some changes to the toolbar flickering? I wasn't able to reproduce the flickering myself at first glances.

See Drupal 10.1.0 introduced new inline JavaScript in the toolbar module to prevent flickering → .

This patch still needs 'jquery cookie library' usage to be replaced by 'js cookie library'.

I was working on this but noticed that there is already an open merge request for this, linked to another issue.

The MR does have this js library change and more, so maybe it's better to continue there?
https://www.drupal.org/project/toolbar_anti_flicker/issues/3410774 📌 Drupal 10 compatibility fixes Fixed

Maybe this can be closed to avoid confusion for others.

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

The submodule's info.yml file was not D11 ready yet.
Also mailsystem is ready for D11 now.

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

The patch applies nicely with "2.x-dev" and "2^" .

It just adds core version requirement for ^11, so the module passes upgrade_status review.

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

Frederikvho → created an issue.

Hi,
I ran into the same warning when editing a node with an HMS field using claro on D10.
Can confirm this patch fixes the notice.

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

Hi,

The configuration section is missing and the introduction heading is not needed, as per my merge request comment above.

The headings are not supposed to be uppercased. Only the first letter of the headings can be capitalized.

View the documentation on README.md templates for examples:
README.md documentation →

Hi apaderno,

I fixed the 57 characters line.

Also changed the documentation because it had both a whitespace and no whitespace for the specific part that kenyoOwen highlighted.
I hope this is okay.

Thanks

Hi kenyoOwen,

Thanks. This was unclear for me as the documentation had both variations with and without a white line there. I have just pushed a white line in the latest commit.

I also updated the documentation for this, as others may face the same confusion and overhead.
https://www.drupal.org/node/2181737/revisions/view/13282134/13298418 →

Hi,

I added a whitespace line as the earlier documentation above does have a whitespace for this specific section:
"This module requires the following modules:" 

I am not sure if this is okay, but two different styles of spacings could lead to confusion.

Thanks @apaderno for this catch, once again.
I fixed it in the latest commit, for MR1

I missed that, sorry. Fixed in latest commit. Please re-review the MR. Kind regards

I added a few whitespaces according to the rules on the Drupal README.md documentation page.
Please review again.
Kind regards.

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

I removed the trailing whitespace. Please review !MR3.

'The supporting organization' section is not required, right? As it's in the module settings already.

Kind regards

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

The trailing whitespace on line 1 should be removed now.
Please check !MR1

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

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

I removed the trailing whitespaces and changed the table of contents heading.
The required parts were already in the README.md.
Sorry for the many commits.
Please review the MR again.

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

Hi @apaderno,

Thanks for that catch and good to know.
I fixed it in the latest commit to the MR.

Hi,

I reviewed all the merge request commands and re-reviewed the README.md file. It looks fine now.
Moving to RTBC.

I made some small changes:
Moved features block below the table of contents.
Added missing whitespace and removed redundant whitespace at the end of the file.
Updated install and requirements segments to be more in line with the Drupal README.md guidelines.

Please review.
Kind regards.

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

Dear @Gisle,

I would like to add to ressa's #2 comment and to point out I just had a similar interaction with this other Specbee employee.

https://www.drupal.org/project/webform_all_download/issues/3321717#comme... 📌 Replace README.txt with README.md Fixed

I also e-mailed you about this. Sorry, only noticed just now that this drupal issue already covers both employees.

Kind regards.

Hi @Nupur Badola,

You seem to be repeating comment #7

Please take a look at the more recent merge request.
As I mentioned in comment #10 the required README.md paragraphs were added in the issue fork.

If you still think the issue needs work, please change the label to 'Needs work'.

Regarding the last comment.

While it is my personal goal to deliver good maintainership for this module.
I want to clarify that my colleagues are also eager to assist me on this journey. Additionally, my employer supports some of the contribution time. With this combined effort, we aim to guarantee a consistent level of maintainership for the module.

So I hope I was not overstepping a boundary by making that update to the project page. It was my understanding that this project was actually looking for a maintainer and not a co-maintainer.

As was also discussed in this issue:
https://www.drupal.org/project/node_title_help_text/issues/3366044 📌 Node title help text appears to be unsupported Fixed
Starting from comment #7

Are you saying this is no longer the case?

Nevertheless, I will still contribute. Just wanted to address my side of the confusion.

Hi Gisle,

Thank you for your recent comment. I appreciate your attention to the matter, a lot.

I, too, share the concern you stated in comment #4 (and #6).

Kind regards

Hi,

It was actually the applying process of the patches that resulted in whitespace errors and not the running of a code sniffer.

Nevertheless, you are saying that these fixes to the documentation had already been taken care of in another issue? Just double checking.

Kind regards

Hi,

As stated in the previous comments, the required paragraphs were added to the README.md file in an issue fork.
Please review the merge request. Thanks!

Hi,

I had the same remarks as #7 and had already pushed fixes to the forked branch.

However, I believe something went wrong creating the fork like I normally do and I was unable to create a merge request for it.

I will try to recreate it in a few hours.

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

Hi,

I still encountered plenty of whitespace errors while applying the patch, so I ran it again using the --whitespace=fix option.
I have created an issue fork and opened a merge request.

The issue should be fixed now. Please review the merge request.

Thanks!

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

Hi,

As I still received a few remaining PHPCS errors I created an issue fork and opened up a merge request.
Summary: Applied the patch from comment #4 and fixed the remaining PHPCS errors.

Please review.
Thanks!

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

Hi,

I created a fork and a merge request for ease of use.
Summary:

• Applied path from comment #8
• Fixed one remaining phpcs remark regarding a whiteline
• Fixed remarks from Apadarno's comment #11

I would like to add a remark that the comments in the 'curious_chronicles_preprocess_page' hook seem to be adding noise for me. Shouldn't they go in a README.md instead?

Please review the opened merge request.

Kind regards,
Frederik

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

Hi,

I applied the patch, ran the code sniffer command again, and I get no new PHPCS errors or warnings. So the last patch is fine.
Thanks.

Hi,

I fixed the two white spaces from the previous warnings in comment #18.
Please review and try to apply the latest .diff from the MR.

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

Hi,

I applied the patch and ran the PHPCS command but it threw one remaining PHPCS error.

So I created a fork, applied the patch from comment #2 there and a fix for the remaining PHPCS error.

Please see the merge request: MR

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

Hi,

I applied the patch from comment #2 and ran PHPCS again.
I did not receive any warnings/errors.

(base) ➜  amazon_sns git:(8.x-1.x) ✗ phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml,twig .
(base) ➜  amazon_sns git:(8.x-1.x) ✗

Hi,

I applied the patch from the issue summary and ran PHPCS again but still received some errors/warnings.

I created a fork, applied the patch there, applied fixes from PHPCBF and applied some additional manual fixes like removing unused variables.
Please review: MR 1

Kind regards

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

Hi,

I applied the patch from comment #2 and ran PHPCS again and it didn't throw any warnings/errors.

(base) ➜  best_selling_products git:(8.x-3.x) ✗ phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml,twig .
(base) ➜  best_selling_products git:(8.x-3.x) ✗

Kind regards,
Frederik

Hi,

I applied the patch from the issue summary and ran PHPCS again. It threw no more warnings/errors. So this looks good.

 (base) ➜  link_selection_handler git:(1.0.x) ✗ phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml,twig .
(base) ➜  link_selection_handler git:(1.0.x) ✗

Hi,

I applied the patch from #2 and ran PHPCS again but it threw 2 remaining errors. I fixed them using PHPCBF and pushed all of it to a fork.
Please see merge request !2 for the latest fixes.

Kind regards,
Frederik

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

Hi,

I applied the patch from the issue summary but it still threw quite a few warnings and errors.

So I created a fork and applied the patch on there, also ran phpcbf to fix some additional errors automatically.

There still are extra PHPCS errors left. I can't fix those right now as I have no environment ready to test all the variables that have to be changed to lowerCamel. But feel free to continue on the fork, if I haven't already.

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

Hi,

I ran the PHPCS command again with this patch and I no longer see any warnings/errors.

(base) ➜  pf_alerta git:(2.2.x) ✗ phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml,twig .
(base) ➜  pf_alerta git:(2.2.x) ✗

Hi

I ran PHPCS again with this patch and there are no PHPCS warnings/errors left.