🇧🇪Belgium @Frederikvho

Should be fixed in to 8.x-1.0-rc5

Added to 8.x-1.0-rc5

frederikvho → created an issue.

Redundant in favor of #3429914

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

Thanks for the patch!
It was added in the latest release: 8.x-1.0-rc4

Thanks for the quick fix!
Added it to the next release:
8.x-1.0-rc4

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

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

Thank you everyone, this is in the latest release 8.x-1.0-rc3

Thank you everyone, this is in the latest release 8.x-1.0-rc2.

Thank you everyone, this is in the latest release 8.x-1.0-rc2.

Thank you everyone, this is in the latest release 8.x-1.0-rc2.

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

Thank you, both of you.
I will schedule some time to go through the issues, soon.

Hi @avpaderno,

Am I still allowed to reopen this and proceed with step #4?
I must admit I was waiting for activity on this issue for some time first but then let it slip out of my radar, sorry.

The project could still benefit from a D11 release, among other things, and I would still be happy to help out.

Kind regards,
Frederik

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.