Account created on 28 March 2009, over 15 years ago
#

Merge Requests

More

Recent comments

πŸ‡«πŸ‡·France DYdave

Hi Ankit (@ankitv18),

I cloned your changes to a separate branch:
https://git.drupalcode.org/issue/token-3157138/-/tree/3157138-gitlabci-f...

Rolled back MR!76 and the CSpell tests seem to be passing again now βœ…
see pipeline:
https://git.drupalcode.org/issue/token-3157138/-/pipelines/222961

Let's hope a maintainer could help us get these changes merged in first, since all CSpell errors are fixed and the job completes successfully.

Then perhaps we could consider making other changes to the project in other tickets.
Thanks!

πŸ‡«πŸ‡·France DYdave

It's not really helping @ankitv18 ...

Aren't there enough tickets, bugs and issues going around where you could really be fixing something instead of trying to mess up with a ticket which was fine and didn't really need help anymore?

There are plenty of helpful ways you could be earning credits if that's what you're looking for, but personally I'm really not convinced any of the changes you've done to this MR add any value to this ticket...

πŸ‡«πŸ‡·France DYdave

@ankitv18:

Why did you change that Ankit?
https://git.drupalcode.org/project/token/-/blob/43a454040745db69b227ccf8...

Do you think it's better to display cspell disable in the README file?

Moving maintainers to a specific file is a "legit" change and the file is explicitly excluded from CSpell validation on GitlabCI...

Personally, I don't think showing cspell disable on the README page would be acceptable...
I would be in favor of reverting this change:
https://git.drupalcode.org/project/token/-/merge_requests/76/diffs?commi...

Not sure if the other change is really needed, but if it is then fine.

πŸ‡«πŸ‡·France DYdave

PHPUnit tests seem to be failing for other reasons...
See failing tests for 4.0.x:
https://git.drupalcode.org/project/s3fs/-/pipelines/220039

Let me know if you'd like this particular error to also get added to tests.

Thanks in advance for your feedback and reviews!

πŸ‡«πŸ‡·France DYdave

Thanks a lot everyone for your great help on this issue and sorry for the delay of this reply.

Quick follow-up:
Easily rebased MR!11 and started running the tests locally on D10.
I quickly noticed the fixtures were based on themes bartik and seven which have been removed in core D10 and moved to contrib.

After adding the themes locally with composer, the tests ran successfully without any change to the work from #4!
Great job getting some tests added!

Lastly, for D11 (phpunit next major), adding the bartik and seven themes with composer was not an option since they don't seem to have compatible versions at this moment, see:

 
Therefore, the D7 Migrate Kernel Tests had to be disabled/excluded temporarily for D11.
 
The changes have been merged in the 8.x-1.x branch at #12, thus marking this issue as Fixed for now.
 
Feel free to let us know if you have any questions, suggestions or concerns on any of the recent code changes or this ticket in general, we would be glad to help!
 
Thanks again everyone for the great work and contributions!

πŸ‡«πŸ‡·France DYdave

DYdave β†’ made their first commit to this issue’s fork.

πŸ‡«πŸ‡·France DYdave

Quick follow-up on this issue:

Support for Drupal 11 was added to the 8.x-1.x branch at #6.

Please let us know if you have any questions, we would be glad to hear your feedback.
Thanks in advance!

πŸ‡«πŸ‡·France DYdave

DYdave β†’ made their first commit to this issue’s fork.

πŸ‡«πŸ‡·France DYdave

Bumping to D11 for 8.x-1.x.

πŸ‡«πŸ‡·France DYdave

Quick follow-up on this issue:

Fixed all the points listed in the issue summary at #2 in merge request MR!43.

Except:

  • Create new legacy compatible development branch: 1.0.x
  • Create stable release (1.0.0) and also set recommended (maintained)

 
It's probably more convenient sticking with the current 8.x-1.x development branch for now and a stable release will be created after support for D11 is added.

Otherwise, at this point: all tests seem to be passing on the 8.x-1.x branch.

Therefore, the tests configurations on DrupalCI for the 8.x-1.x branch were removed:
https://www.drupal.org/node/246513/qa β†’

The only tests remaining are for the D7 branches of the module.

Feel free to let us know if you have any questions or concerns on any aspects of this ticket or the latest changes in general, we would surely be glad to help.
Thanks in advance!

πŸ‡«πŸ‡·France DYdave

Thanks everyone for your help and contributions!

Rebased the merge request at #8 and since the tests were still passing and the refactoring change made sense, MR!15 was merged at #9.

Marking issue Fixed for now.
Thanks!

πŸ‡«πŸ‡·France DYdave

DYdave β†’ made their first commit to this issue’s fork.

πŸ‡«πŸ‡·France DYdave

Thanks a lot Tim (@TR)!
Great job on creating the new major branch and all your updates in other tickets!

Now this issue is fixed in 4.0.x, we will get back to working on related honeypot ticket to pass the tests.
When I get a chance, I'll try taking a look at the Drush related ticket as well.

Thanks again!

πŸ‡«πŸ‡·France DYdave

Quick follow up on this issue:

Fixed all PHPSTAN validation errors and added Functional test cases for every class or form impacted:

  • Block configuration form,
  • Admin settings form and
  • Bulk operations forms

 
Feel free to let us know if you have any questions or conerns on any aspects of this ticket or the project in general, we would be glad to help.
Thanks in advance!

πŸ‡«πŸ‡·France DYdave

Quick follow-up on this issue:

All the tasks listed in the IS have been completed.

The release image_link_formatter:2.2.0 β†’ is now available for download and update.

Feel free to let us know if you have any questions or concerns on any aspects of this issue or the project in general, we would be glad to help.
Thanks in advance!

πŸ‡«πŸ‡·France DYdave

Thank Tim (@TR) once again for your prompt and very clear follow-up on this issue.

Feel free to let us know if there is anything we could do to help, re-rolls, rebase, or any other change that would be required in the merge request.

Thanks in advance !

πŸ‡«πŸ‡·France DYdave

Rerolled the patch from #21 to apply to 10.2.7.

The patch seems to fix the issue.

Thanks everyone for the help!

πŸ‡«πŸ‡·France DYdave

For those ending up landing like me on this issue after a search for:
ddev storybook stuck <i> [webpack-dev-middleware] wait until bundle finished
getting stuck during the ddev installation process with webpack5 and (project) type=html, as described on the ddev-storybook plugin page:

I managed bypassing the problem by using Vite instead, which allowed completing the app setup without any further error or obstacle.

Eventually, I got to installing the Storybook β†’ module, following the setup instructions at:
https://git.drupalcode.org/project/storybook/-/blob/1.x/README.md

Which lead to further executing a few more commands with Yarn, probably also adding more changes to the package.json and storybook config files, in particular, changing the project type to server.

Overall, the storybook app seems to be working well, along with the module, on a ddev stack:

  • ddev:v1.23.1
  • storybook:8.1.6
  • drupal/core:10.2.6
  • drupal/storybook:1.0.0-beta2
  • node:v20.13.1
  • npm:10.5.2
  • yarn:4.2.2

 
Lastly, one of my colleagues more experienced in front-end development, explained he preferred working with Yarn instead of npm, which also seems to be the recommended method described in the storybook module README file.

Thanks everyone for the help, comments and documentation!

πŸ‡«πŸ‡·France DYdave

Hi Tim (@TR),

Just wanted to mention I've come across similar issues with deprecated code or functions in newer versions of Drupal Core.

It's probably not the first time you've faced this but I'd like to share what I was able to find on the topic:

If it helps, I found the following posts:
https://mglaman.dev/blog/drupal-module-semantic-versioning-drupal-core-s...
https://www.drupal.org/project/ideas/issues/3357742 🌱 Guidelines for semantic versioning and Drupal core support Active

Basically, accordingly to Matt's blog post, since the current version of the module is already compatible with D10 (Major), it would make sense to create a new minor development branch, in this case 3.0.x or 3.1.x which would drop support for D10.1, thus support D10.2 and above.

If you'd like to keep backward compatibility in the same development branch, we could always modify the patch to add cases on \Drupal::VERSION and have code for previous and future versions in the same file, for example, see what we've done for block_class:2.0.x:
https://git.drupalcode.org/project/block_class/-/blob/2.0.x/src/Controll...
or in tests:
https://git.drupalcode.org/project/block_class/-/blob/2.0.x/tests/src/Fu...
and it works ==> Tests fixed with a broad coverage, see pipeline:
https://git.drupalcode.org/project/block_class/-/pipelines/191822
with multiple Drupal core versions being tested/supported.

This code is mostly inspired from the admin_toolbar module, which tends to be very strict and careful with backward compatibility given the popularity of the module and probably the wide range of Drupal core versions using it.

However, as soon as the current block_class:2.0.x branch is stabilized and we've fixed more major or critical issues, I believe the correct way to move foward would be to create a new minor release development branch which will drop support for 10.1, just like it is suggested in Matt's post.
See also a very similar problem we've been facing with a merge request for the block class module #3412155-11: Deprecated system_get_module_admin_tasks in drupal:10.2.0 and is removed from drupal:11.0.0 β†’ .

Probably the same comment stands for the related Honeypot issue πŸ“Œ PHPUnit: Fix Functional Tests 'HoneypotFormCacheTest' Needs review and probably any change related to code deprecation and backward compatibility in general.

These types of issues are very tricky and definitely not as straight forward as we could think.

If you have other ideas, advice or suggestions, I would be very happy to hear your feedback, since I'm also maintaining quite a few modules which necessarily come across the same type of code deprecation issues.

Feel free to let us know if you have any questions, suggestions or if we could do anything to help, we would surely try answering as soon as possible.
Thanks in advance!

πŸ‡«πŸ‡·France DYdave

Thanks @solideogloria!

The stable release for the 2.0.x branch has just been marked recommended βœ…

Let us know if you spot anything else.

Thanks again for your help testing and reporting!
Cheers!

πŸ‡«πŸ‡·France DYdave

This issue is for the 2.0.x branch which currently supports D10.

Marking it as Fixed for now.

Thanks everyone for the help testing and reporting on this issue.

πŸ‡«πŸ‡·France DYdave

Hi Manish (@manish-31),

Thanks a lot for the merge request and code contributions.

According to the change record: system_get_module_admin_tasks() is deprecated β†’ as of D10.2.0 where the new service system.module_admin_links_helper was introduced and currently used in your merge request:
https://git.drupalcode.org/project/block_class/-/merge_requests/37/diffs...

If we were to merge these changes we would be breaking backward compatibility (BC) and this feature would not work with D10.1, D10.0 or D9, which means we would have to drop support for 10.1 and only support 10.2 and above.

In order to get MR!37 merged, we're going to have to create the new minor release branch 2.1.x which will drop support for D10.1.

For the time being, the error mentioned in the issue summary has been fixed in related ticket πŸ› Fix fatal 'TypeError: ModulePermissionsLinkHelper::getModulePermissionsLink(): Argument #2 ($name) must be of type string, array given' Fixed with changes committed to the 2.0.x branch.

We should be able to come back to this ticket soon... once we've got other non breaking changes added to the current 2.0.x branch.

Feel free to let us know if you have any questions or concerns on this comment or the ticket in general, we would surely be glad to help.
Thanks!

πŸ‡«πŸ‡·France DYdave

Quick follow-up on this issue:

A few additional commits were added to the current merge request MR!41 with a few comments, see above at #4.

Mostly:

  • Fixed fatal TypeError with compatibility support for 10.2 and above without breaking backward compatibility
  • Broadened Gitlab CI tests coverage.
  • Fixed compatibility tests for D9.
  • Added initial Functional tests class for the admin Help page.

 
The tests all seemed to pass for the MR so the changes have been merged in branch 2.0.x, which seems to still be passing 🟒
https://git.drupalcode.org/project/block_class/-/pipelines/191822
(for the different Drupal versions tested)

Marking issue as Fixed for now.
Thanks!

πŸ‡«πŸ‡·France DYdave

Hi Rajdip (@rajdip_755)!

Thanks for your prompt follow-up on this!

Watchout: This is much trickier than it seems ⚠️

We're trying to support multiple versions of Drupal Core for the module in the 2.0.x branch, in particular D9 and D10.

The current code works for D9, D10.0, D10.1 and breaks as of D10.2.

We're currently trying to figure out what's the best option moving forward... probably creating a new branch which would most likely drop support for D10.1 (so only supports D10.2 and above).

We've mostly got the fix for D10.2 already in merge request MR!37, see related issue πŸ› Deprecated system_get_module_admin_tasks in drupal:10.2.0 and is removed from drupal:11.0.0 Needs review .

Mostly this ticket is to fix the module for D9, D10.0 and D10.1.
Once we've got that fixed, we should be able to get back to πŸ› Deprecated system_get_module_admin_tasks in drupal:10.2.0 and is removed from drupal:11.0.0 Needs review , create a new minor development branch and get MR37 merged in.

I'm currently working on this issue, fixing phpunit tests and broadening test coverage.

Feel free to let us know if you have any questions or concerns on any aspects of this issue or the project in general, we would surely be glad to help.
Thanks in advance!

πŸ‡«πŸ‡·France DYdave

Super nice of you Erik (@Erik Seifert)! Thanks a lot!

I've just checked and the ticket appears correctly on the company page!
My colleagues will be pleased πŸ™‚

Thanks again Erik for your reactivity and great help fixing this issue, it's super appreciated!
Cheers!

πŸ‡«πŸ‡·France DYdave

Thanks a lot Eric (@ericgsmith) for your comment at #30!

It provided the basic guidelines for the implementation of this feature:

I've created a new merge request MR!28 with the approach suggested with a new configuration form setting additional_form_ids which could be used in the service.

Additionally, a few tests were added to check the additional custom form ids would be properly protected by honeypot, see:
https://git.drupalcode.org/project/honeypot/-/merge_requests/28/diffs#di...

 
The added test cases seem to pass.
The only errors currently reported are not related with this merge request, they have already been addressed in different issues, see:

 
Therefore, at this point, this new feature "should" be ready to be tested, thus switching ticket back to Needs review as an attempt to get more reviews, testing and feedback on MR!28.

Thanks in advance!

πŸ‡«πŸ‡·France DYdave

Thanks a lot @nils.destoop for your prompt follow-up on this and for the extra clean-up!
Cheers!

πŸ‡«πŸ‡·France DYdave

Thanks a lot Paulo (@paulocs) for creating this issue and contributing a merge request, it's particularly appreciated!

With the link you provided to the Change record: _system_rebuild_module_data() and co. are replaced by services to give you all available modules and themes β†’
along with the details you provided, completely makes sense.

See for example, current module's code:
https://git.drupalcode.org/project/block_class/-/blob/2.0.x/src/Controll...
The module would certainly crash with older versions < 8.7.

This has been the case for a while in tagged releases : I went back to 2.0.0 and it had the same code... So it is safe to assume this change isn't going to have much impacts on sites, since they would have had to stop upgrading a while ago (to avoid crashing their site).

The changes have been merged!

The module is no longer supported or maintained for Drupal 8:
If you would like to keep upgrading to the latest version of the module, you will have to upgrade to drupal 9.

Thanks again for your help!

πŸ‡«πŸ‡·France DYdave

DYdave β†’ made their first commit to this issue’s fork.

πŸ‡«πŸ‡·France DYdave

Hi Todd (@Todd Nienkerk),

Could you please help us change the default branch of the module to the current 2.0.x in the Gitlab repository?

It is starting to become particularly painful and costly having to rebase merge requests on Gitlab manually.

Most of the merge requests received now in the Gitlab repo are targeting the 2.0.x branch.
But since it isn't currently the default branch configured, Gitlab is unable to properly automatically rebase the merge requests through the interface and requires additional manual work.

Although I have the chance to be one of the maintainers of the block_class module, I don't seem to have the required level of permissions to access the Maintainers tab and Settings in the project's Gitlab repo at:
https://git.drupalcode.org/project/block_class/-/settings/repository
(I'm getting a 404)
Otherwise I would have done it myself already πŸ˜…

See the very clear DO documentation: Maintainership: Setting a default branch β†’
for the detailed steps to access the repo's settings form and change the default branch, which should only take 5 minutes.

I think this change/operation might require the user marked as the author of the module and that's why we are contacting you directly Todd (@Todd Nienkerk), but if anyone else among module maintainers would have that kind of access, some help would definitely be greatly appreciated.

Lastly, if there is any particular reason the current default branch 8.x-1.x shouldn't be updated, please let us know and maybe we will try finding other options or solutions, but in the current state it doesn't really seem viable in terms of the amount of extra work it creates for all of us, whether creating MRs, rebasing, etc...

Bumping ticket to Major as an attempt to attract maintainers attention: Too much extra work maintaining merge requests.

Feel free to let us know if you have any questions or concerns on this comment, or the ticket in general, we would certainly be happy to help.
Thanks in advance!

πŸ‡«πŸ‡·France DYdave

Thanks everyone for creating this ticket and your code contributions, it's greatly appreciated!

I've taken a quick look at this issue and tried repeating the steps described in the IS and at #5 but unfortunately I was unable to reproduce the problem and prompt the PHP error message:
βœ… No crash: Everything worked fine/as expected

Tested with:

  • drupal/core: 10.2.5
  • drupal/block_class: 'dev-2.0.x:e03fb6a61296eb8ae595bfc8298e33e633a57610'

 
Step 1: Install module block_class from scratch (no prior config):

> drush en block_class
 [success] Successfully enabled: block_class

> drush cget block_class.settings

_core:
  default_config_hash: XEpuHrBp3DELKxVx0tmwE6CS7NSIKXu4F6QhT_vwqd0
block_classes_stored: {  }
default_case: standard
enable_attributes: true
enable_auto_complete: true
enable_id_replacement: true
enable_special_chars: false
field_type: multiple_textfields
filter_html_clean_css_identifier: ''
items_per_page: 50
qty_attributes_per_block: 10
qty_classes_per_block: 10
maxlength_attributes: 255
maxlength_block_class_field: 255
maxlength_id: 255
weight_attributes: 0
weight_class: 0
weight_id: 0

As you can see block_classes_stored: { } is an array, so it seems it "should" have an empty array [] as its default value and not null.

Additionally, is seems auto-complete is enabled by default as well: enable_auto_complete: true.

See current: block_class.settings.yml
https://git.drupalcode.org/project/block_class/-/blob/e03fb6a61296eb8ae5...
 

Step 2: Browse to "Main page content" (system_main_block) edit form:
/admin/structure/block/manage/olivero_content
 

Step 3: Add a class in autocomplete field "CSS class", for example: 'testingBC1', then save.
 

I'm not sure if I'm missing anything here, but with a fresh 'drupal/block_class:2.0.x-dev@dev' module install I was not able to reproduce the error.
 
Could someone please take another look at this issue and let us know if the problem is still affecting the latest 2.0.x DEV code base?

For reference, it seems the default value of the block_classes_stored config was updated in ✨ Storing block_classes_stored as a string value leads to poor developer experience Fixed .
 

Lastly, I'd like to point out the steps outlined above should actually be the same ones executed in module's Functional tests for the Create ("add") block form: /admin/structure/block/add/system_main_block/olivero.
which all seem to be passing at the moment: 🟒

Code in test class:
https://git.drupalcode.org/project/block_class/-/blob/e03fb6a61296eb8ae5...
 

We would greatly appreciate if anyone could help confirming the problem in this ticket has been resolved in current DEV, or if it is still active, in which case, more contextual information would be very helpful.

Thanks in advance for your tests, reporting, comments and feedback.

πŸ‡«πŸ‡·France DYdave

DYdave β†’ made their first commit to this issue’s fork.

πŸ‡«πŸ‡·France DYdave

This was fixed in πŸ“Œ Fix the issues reported by phpcs Fixed and committed to the 2.0.x branch.

Thanks everyone for the great work on this issue.

πŸ‡«πŸ‡·France DYdave

The changes have been merged!

Build pipeline passed: https://git.drupalcode.org/project/block_class/-/pipelines/186710
PHPCS job passed: https://git.drupalcode.org/project/block_class/-/jobs/1734340

Thanks everyone for the great help and work!

πŸ‡«πŸ‡·France DYdave

DYdave β†’ made their first commit to this issue’s fork.

πŸ‡«πŸ‡·France DYdave

At this point:
Last build on MR!40: https://git.drupalcode.org/project/block_class/-/pipelines/186665

The changes have been merged!

Thanks!

πŸ‡«πŸ‡·France DYdave

Thanks a lot for creating this issue and contributing a merge request, it's greatly appreciated!

The changes have been merged!

The module is now integrated with Gitlab CI, see latest build on the 2.0.x branch:
https://git.drupalcode.org/project/block_class/-/pipelines/186653

Thanks everyone!

πŸ‡«πŸ‡·France DYdave

Thanks a lot @paulocs for the great help and work on this issue!

In the end, it was preferred to go with olivero instead of stable9 (suggested in MR!33#3df28a9e in πŸ“Œ Add Gitlab CI Needs review ) or stark (suggested in this ticket's MR!19) since they don't add classes on markup.

In this test, ideally, we'd like to confirm the classes added by other components don't get overridden/replaced.
So in other words: test that classes that were supposed to be added are still there after the block_class classes are added, see:
https://git.drupalcode.org/project/block_class/-/merge_requests/39/diffs...
In particular: class="TestClass_content block block-system block-system-main-block".

Changes merged!

Thanks again!

πŸ‡«πŸ‡·France DYdave

DYdave β†’ made their first commit to this issue’s fork.

πŸ‡«πŸ‡·France DYdave

Thanks everyone!

In the end we preferred sticking with an integer value and just change its default from false to 255, like other maxlength config fields.
It appears to simplify slightly the code while keeping the same logic and results.

Added update hook to set the value to 255 if it is currently empty.

The changes have been merged!

Thanks!

πŸ‡«πŸ‡·France DYdave

DYdave β†’ changed the visibility of the branch 3413783-make-maxlengthattributes-nullable to hidden.

πŸ‡«πŸ‡·France DYdave

DYdave β†’ made their first commit to this issue’s fork.

πŸ‡«πŸ‡·France DYdave

Changes merged!

These changes were also included in larger merge request MR!19 from issue πŸ“Œ Update the BlockClassTest class to work on 2.x branch Needs review .

Thanks everyone!

πŸ‡«πŸ‡·France DYdave

Thanks a lot Tim (@TR) for your prompt reply and for finding the corresponding change record.

As suggested, I went ahead and updated the version requirement of the module to "drupal/core": "^10.2".

As a follow-up task, I've added πŸ“Œ Support drush ^12 commands Active , looking at the line "drush.services.yml": ">=9" in the composer.json, that could most likely be removed, since drush ^12.4.3 would now be required.

Once again, we would greatly appreciate your reviews, advice and feedback.

Feel free to let us know if you would see anything else or more work needed on the merge request MR!26 or the issue in general, we would be happy to help.
Thanks!

πŸ‡«πŸ‡·France DYdave

@marcoliver, when you get a moment, could you please change the target branch of the merge request to 11.x, as requested at #8 ?
It seems I'm unable to edit the merge request, even with push access to the issue fork.

Otherwise if we need to create a new MR, let me know I would be glad to do so.
Thanks!

πŸ‡«πŸ‡·France DYdave

Not sure whether ticket's status should be changed back to Needs review, but the last comments have been answered.

Additional reviews, comments and feedback would be greatly appreciated.
Thanks in advance !

πŸ‡«πŸ‡·France DYdave

Quick follow-up on this ticket:

A few changes were added to the current merge request MR!27 with a few comments, see above at #2.

But mostly, at this point:
Last build on MR!27: https://git.drupalcode.org/issue/honeypot-3450488/-/pipelines/184573

PHPUnit job: https://git.drupalcode.org/issue/honeypot-3450488/-/jobs/1712394
The 2 issues described above seem to have been fixed and the only remaining issue is the one related with the Rules module fixed in MR:
https://git.drupalcode.org/project/rules/-/merge_requests/26

We would greatly appreciate if a maintainer or someone with write permission could take a look at ticket's merge request MR!27 and let us know if there would be any more work needed.

At this point, i's really up to the maintainers:

  • whether to go ahead with this merge request, although the rules test is failing,
  • or whether the remaining failing test should be disabled temporarily in the same MR (so they all pass).

In any case, feel free to let us know if you have any questions or concerns on any of the changes in the merge request or this ticket in general, we would surely be happy to help.
Thanks in advance for your feedback and reviews

πŸ‡«πŸ‡·France DYdave

Thanks a lot Tim (@TR)!

Based on your instructions I was able to take an initial stab at this issue and the changes are now ready to be reviewed in ticket's merge request MR!26.

In the last build:
https://git.drupalcode.org/issue/rules-3425542/-/pipelines/184562

The issue seems to have disappeared from the PHPUnit job:
https://git.drupalcode.org/issue/rules-3425542/-/jobs/1712331

Taking the failing tests down from 124 to 3 πŸŽ‰ \o/

I haven't looked at the remaining errors yet, but assume they wouldn't necessarily be "too" difficult to fix.
If you think it would be worth looking into them and try fixing them as well in the same MR, please let us know.

Otherwise, I suppose this merge request should fix at least the issue describe in this ticket.

We would greatly appreciate if you could please try testing, reviewing the changes and give us your feedback.

Feel free to let us know if you have any questions or concerns on any of the changes in this merge request or the ticket in general, we would certainly be happy to help.
Thanks in advance!

πŸ‡«πŸ‡·France DYdave

DYdave β†’ made their first commit to this issue’s fork.

πŸ‡«πŸ‡·France DYdave

Sorry about that: I got a bit confused here with the number of merge requests in this ticket and didn't really understand what had been done...

It appears the merge request that I modified above (#59 only fixes part of the issues : the ones related with the doc comments blocks mostly, see :
https://git.drupalcode.org/project/token/-/merge_requests/58/diffs#diff-...

As per #49 could someone please help creating some kind of "binder", with all the merge requests from this ticket merged into a single one, without a phpcs.xml file?
(without excluding any error from validation)

Maybe checking out each branch locally and merging it into a new one ("binder") would allow retaining every commit that was made for every merge request created in this issue.
There will mostly likely be some conflicts to be fixed as well when the various merge requests are combined.

Any help or feedback would be greatly appreciated.

Thanks in advance !

πŸ‡«πŸ‡·France DYdave

Quick follow-up on this issue:

A few additional commits were added to the current merge request MR!58 with a few comments, see above at #58.

But mostly, at this point:
Last build on MR!58: https://git.drupalcode.org/issue/token-2774071/-/pipelines/181772

 

These changes should be harmless enough, without any change in module's code (only doc comments) so they shouldn't take too long to review and would help enforcing validation of coding standards for other pending merge requests.

We would greatly appreciate if a maintainer or someone with write permission could take a look at ticket's merge request MR!58 and let us know if there would be any more work needed.

Feel free to let us know if you have any questions or concerns on merge request MR!58 or any aspect of this ticket in general, we would surely be glad to help.
Thanks in advance for your feedback and reviews.

πŸ‡«πŸ‡·France DYdave

DYdave β†’ changed the visibility of the branch 2774071-fix-comments-debug1 to hidden.

πŸ‡«πŸ‡·France DYdave

DYdave β†’ changed the visibility of the branch 2774071-left-violations to hidden.

πŸ‡«πŸ‡·France DYdave

DYdave β†’ changed the visibility of the branch 2774071-use-fix-empty-line-fix to hidden.

πŸ‡«πŸ‡·France DYdave

DYdave β†’ changed the visibility of the branch 2774071-fix-cs-squiz to hidden.

πŸ‡«πŸ‡·France DYdave

DYdave β†’ changed the visibility of the branch 2774071-null-coalesce to hidden.

πŸ‡«πŸ‡·France DYdave

DYdave β†’ changed the visibility of the branch 2774071-comments-switch to hidden.

πŸ‡«πŸ‡·France DYdave

DYdave β†’ changed the visibility of the branch 2774071-visibility-array-indention to hidden.

πŸ‡«πŸ‡·France DYdave

DYdave β†’ changed the visibility of the branch 2774071-fix-cs-base-branch to hidden.

πŸ‡«πŸ‡·France DYdave

DYdave β†’ made their first commit to this issue’s fork.

πŸ‡«πŸ‡·France DYdave

Quick follow-up on this issue:

A few additional commits were added to the current merge request MR!59 with a few comments, see above at #2.

But mostly, at this point:
Last build on MR!59: https://git.drupalcode.org/issue/token-3411331/-/pipelines/181703

 

We would greatly appreciate if a maintainer or someone with write permission could take a look at ticket's merge request MR!59 and let us know if there would be any more work needed.

Feel free to let us know if you have any questions or concerns on merge request MR!59 or any aspect of this ticket in general, we would surely be glad to help.
Thanks in advance for your feedback and reviews.

πŸ‡«πŸ‡·France DYdave

Closing in favor of more recent and better documented issue πŸ“Œ Fix eslint violations Needs work .

Thanks!

πŸ‡«πŸ‡·France DYdave

DYdave β†’ made their first commit to this issue’s fork.

πŸ‡«πŸ‡·France DYdave

Since the module is now integrated with Gitlab CI, let's try bringing back this issue at the top of the stack:

See initial CSPELL error report:
https://git.drupalcode.org/issue/token-3157138/-/jobs/1681579
(where we started)
 

Quick follow-up on this ticket:

Created initial merge request MR!76 based on the patch provided at #9, which didn't apply completely anymore, but still managed fixing almost all the errors that were reported by the CSpell job on Gitlab CI.

A few initial commits were added to the current merge request MR!76 with a few technical details, see above at #12.

But mostly, at this point:
Last build on MR!76: https://git.drupalcode.org/issue/token-3157138/-/pipelines/181574

 

These changes are harmless enough, without any change in module's code (only comments) so they shouldn't take too long to review and would help enforcing automated spell check validation for other pending merge requests.

We would greatly appreciate if a maintainer or someone with write permission could take a look at ticket's merge request MR!76 and let us know if there would be any more work needed.

Feel free to let us know if you have any questions or concerns on merge request MR!76 or any aspect of this ticket in general, we would surely be glad to help.
Thanks in advance for your feedback and reviews.

πŸ‡«πŸ‡·France DYdave

DYdave β†’ changed the visibility of the branch 3157138-drupal-9.1.x-code to hidden.

πŸ‡«πŸ‡·France DYdave

DYdave β†’ made their first commit to this issue’s fork.

πŸ‡«πŸ‡·France DYdave

Quick follow-up on this issue:

A few additional commits were added to the current merge request MR!75 with a few comments, see above at #2.

But mostly, at this point:
Last build on MR!75: https://git.drupalcode.org/issue/admin_toolbar-3449577/-/pipelines/181273

 

Added temporary commit to be reverted:
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/75/dif...
ported from πŸ“Œ GitlabCI support: Fix all jobs validation errors and PHPUnit tests RTBC , based on MR MR!68 to pass phpunit tests and add .gitlab-ci.yml file.
Revert this commit once these changes are merged in the development branch.
 

We would greatly appreciate if a maintainer or someone with write permission could take a look at ticket's merge request MR!75 and let us know if there would be any more work needed.

Feel free to let us know if you have any questions or concerns on merge request MR!75 or any aspect of this ticket in general, we would surely be glad to help.
Thanks in advance for your feedback and reviews.

πŸ‡«πŸ‡·France DYdave

Thanks for the prompt feedback.

Agreed: seems too risky to include these changes in the same MR, sounds like a lot to testing in one go and getting CSpell, PHPCS, PHPStan and PHPUnit fixed would already be a great step forward.

The work on ESLint was moved to a separate issue, see: πŸ“Œ GitlabCI: Fix ESLINT validation errors Active , thus reverted all the changes to JS files and eslint configuration for Gitlab CI.
(Changes were mostly cherry-picked to be moved to a different branch/issue fork)

At this point, all tests now seem to be passing green 🟒 (Except ESLint, to be addressed in a separate merge request).
https://git.drupalcode.org/issue/admin_toolbar-3407845/-/pipelines/181112
 

Feel free to let us know if you have any questions or concerns on merge request MR!68 or any aspect of this ticket in general, we would surely be glad to help.
Thanks in advance for your feedback and reviews.

πŸ‡«πŸ‡·France DYdave

Quick follow-up on this issue:

A few additional commits were added to the current merge request MR!68 with a few comments, see above.

But mostly, at this point:
Last build on MR!68: https://git.drupalcode.org/issue/admin_toolbar-3407845/-/pipelines/180699

 

In order for the tests to pass, the broken core test \Drupal\Tests\toolbar\Functional\ToolbarAdminMenuTest::testSubtreesJsonRequest had to be temporarily disabled, until related issue πŸ“Œ When using drupalGet(), provide an associative array for $headers Needs work is fixed. From a testing standpoint, this failing core test shouldn't be holding up the tests for the admin_toolbar module: Whenever it is fixed in core, it could be re-enabled for the module.
See: https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/68/dif...
 

At this point, all tests now seem to be passing green 🟒\o/ πŸŽ‰
https://git.drupalcode.org/issue/admin_toolbar-3407845/-/pipelines/180699
thus marking issue as Needs review and bumping as Major as an attempt to attract maintainers attention.

We would greatly appreciate if a maintainer or someone with write permission could take a look at ticket's merge request MR!68 and let us know if there would be any more work needed.

Feel free to let us know if you have any questions or concerns on merge request MR!68 or any aspect of this ticket in general, we would surely be glad to help.
Thanks in advance for your feedback and reviews.

πŸ‡«πŸ‡·France DYdave

Thanks a lot Marc-Oliver (@marcoliver) for raising this issue and for your code contributions, it's greatly appreciated!

Just a quick comment to confirm the core module Toolbar test:
Drupal\Tests\toolbar\Functional\ToolbarAdminMenuTest::testSubtreesJsonRequest

currently seems to be failing πŸ”΄ (10.3.x/10.4.x), see:
https://git.drupalcode.org/project/drupal/-/blob/10.3.x/core/modules/too...

$ ./vendor/bin/phpunit ./web/core/modules/toolbar/tests/src/Functional/ToolbarAdminMenuTest.php

PHPUnit 9.6.19 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\toolbar\Functional\ToolbarAdminMenuTest
.....E...                                                           9 / 9 (100%)

Time: 01:23.484, Memory: 4.00 MB

There was 1 error:

1) Drupal\Tests\toolbar\Functional\ToolbarAdminMenuTest::testSubtreesJsonRequest
TypeError: Behat\Mink\Session::setRequestHeader(): Argument #1 ($name) must be of type string, int given, called in /var/www/html/web/core/tests/Drupal/Tests/UiHelperTrait.php on line 235

/var/www/html/vendor/behat/mink/src/Session.php:199
/var/www/html/web/core/tests/Drupal/Tests/UiHelperTrait.php:235
/var/www/html/web/core/modules/toolbar/tests/src/Functional/ToolbarAdminMenuTest.php:387
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:729

ERRORS!
Tests: 9, Assertions: 207, Errors: 1.

 

I haven't had the time to look any further than that or really test the changes from the merge request (MR!7445), but we would definitely appreciate some reviews, testing and feedback.

Thanks in advance!

πŸ‡«πŸ‡·France DYdave

Thanks a lot everyone for all the work on this issue.

One thing though: Why would ESLint be disabled?

I've taken a quick look at the issues raised by the ESLint job:
https://git.drupalcode.org/issue/admin_toolbar-3407845/-/jobs/1291671
They don't look very difficult to fix, especially since 171 errors out of 192 could be automatically fixed with the --fix flag.
(see report linked above)

If it's a question of work and amount of changes, if fixing the ESLint job would take too much work, we could leave it aside/untouched and create a different issue specifically to fix ESLint for the module, see for example: πŸ“Œ GitlabCI: Fix ESLINT validation errors Needs review .

But I would certainly advise against completely shutting down the job, see:
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/68/dif...

variables:
  SKIP_ESLINT: '1'

Switching back to Needs work, as an attempt to get feedback and perhaps an update to the GitlabCI file to revert the ESLint configuration.

Any feedback, comments, suggestions or questions would be greatly appreciated.

Feel free to let us know if you have any questions or concerns on any aspects of this comment or the ticket in general, we would surely be glad to help.
Thanks in advance!

πŸ‡«πŸ‡·France DYdave

@Erik Seifert:

Not sure exactly how this works, but I would greatly appreciate if you could please mark the issue as fixed and potentially credit me for the issue, if possible.

Ideally, I'd like it to appear on my company's page, on whose behalf I was able to contribute all the work in this issue and I'm sure you would understand.

It looks like when I'm marking the ticket as Fixed, I don't seem to be getting any credit, "perhaps" because I'm not a maintainer of the module.

In any case, thanks a lot for your help reviewing and merging the changes from this ticket into module's main development branch.

Thanks in advance!

πŸ‡«πŸ‡·France DYdave

Wow! Super happy about that Eric (@Erik Seifert)!

Thanks a lot for merging the changes!

The module is now passing all green 🟒 \o/ πŸŽ‰
2.1.x: https://git.drupalcode.org/project/dashboards/-/pipelines/179454

Any further merge request or ticket should now be able to validate through all the tests/jobs, preventing any regression from being introduced in the module.

This was pretty much the objective of this issue, therefore, at this point we can consider that all the work to be carried in this ticket has been completed, thus marking it as fixed for now.

Feel free to let us know at any point if you have any questions or would see any additional task that we could have missed, we would certainly be glad to hear your feedback.
Thanks again very much Eric for your prompt and positive feedback.
Cheers!

πŸ‡«πŸ‡·France DYdave

Quick follow-up on this issue:

Since there were not too many issues and some of them were already fixed by the PHPCS changes, I went ahead and got all PHPSTAN errors fixed in the same MR.

At this point PHPCS, PHPSTAN and PHPUNIT jobs all seem to be passing 🟒
See last build: https://git.drupalcode.org/issue/field_group-3423199/-/pipelines/179212

Feel free to let us know if you have any questions or concerns on merge request MR!47 or any aspect of this ticket in general, we would surely be glad to help.
Thanks in advance for your feedback and reviews.

πŸ‡«πŸ‡·France DYdave

The tests seem to be fixed in πŸ“Œ Automated Drupal 11 compatibility fixes for field_group - Fixed PHPUnit on GitlabCI RTBC , in particular with the changes in file contrib/field_group_migrate/tests/src/Functional/MigrateUiFieldGroupTest.php, see:
https://git.drupalcode.org/project/field_group/-/merge_requests/49/diffs...
maybe, @Anybody, it might be worth taking a closer look at this other MR first (MR!49) so phpunit tests could get fixed and run for other tickets.

I would recommend getting the ESLint job fixed first in πŸ“Œ GitlabCI: Fix ESLINT validation errors Needs review (MR!54), before getting this MR approved:
it seems it would introduce quite a few linting errors, mostly in the JS files.

Otherwise, @rcodina, you could try cherry-picking a commit from another MR, I've used to pass the tests, see :
https://git.drupalcode.org/project/field_group/-/merge_requests/47/diffs...
Or simply download the file locally and try running the phpunit tests, then commit the file and re-run the tests.

Hope that helps moving the ticket forward.
Thanks in advance for your feedback!

πŸ‡«πŸ‡·France DYdave

Quick follow-up on this issue:

A few additional commits were added to the current merge request MR!54 with a few comments, see above at #2.

But mostly, at this point:
Last build on MR!54: https://git.drupalcode.org/issue/field_group-3448470/-/pipelines/178635

 

Changes to file MigrateUiFieldGroupTest.php from merge request MR!49 (see πŸ“Œ Automated Drupal 11 compatibility fixes for field_group - Fixed PHPUnit on GitlabCI RTBC ) had to be ported to this MR for the phpunit tests to complete, see:
https://git.drupalcode.org/project/field_group/-/merge_requests/49/diffs...
So changes to this file could probably be reverted once the phpunit tests are fixed in the main development branch (8.x-3.x).
 

We would greatly appreciate if a maintainer or someone with write permission could take a look at ticket's merge request MR!54 and let us know if there would be any more work needed.

Feel free to let us know if you have any questions or concerns on merge request MR!54 or any aspect of this ticket in general, we would surely be glad to help.
Thanks in advance for your feedback and reviews.

πŸ‡«πŸ‡·France DYdave

Quick follow-up on this issue:

A few additional commits were added to the current merge request MR!53 with a few comments, see above at #2.

But mostly, at this point:
Last build on MR!47: https://git.drupalcode.org/issue/field_group-3447953/-/pipelines/176739

 

Changes to file MigrateUiFieldGroupTest.php from merge request MR!49 (see πŸ“Œ Automated Drupal 11 compatibility fixes for field_group - Fixed PHPUnit on GitlabCI RTBC ) had to be ported to this MR for the phpunit tests to complete, see:
https://git.drupalcode.org/project/field_group/-/merge_requests/49/diffs...
So changes to this file could probably be reverted once the phpunit tests are fixed in the main development branch (8.x-3.x).
 

We would greatly appreciate if a maintainer or someone with write permission could take a look at ticket's merge request MR!53 and let us know if there would be any more work needed.

Feel free to let us know if you have any questions or concerns on merge request MR!53 or any aspect of this ticket in general, we would surely be glad to help.
Thanks in advance for your feedback and reviews.

πŸ‡«πŸ‡·France DYdave

Quick follow-up on this issue:

A few additional commits were added to the current merge request MR!47 with a few comments, see above at #6.

But mostly, at this point:
Last build on MR!47: https://git.drupalcode.org/issue/field_group-3423199/-/pipelines/176726

 

Changes to file MigrateUiFieldGroupTest.php from merge request MR!49 (see πŸ“Œ Automated Drupal 11 compatibility fixes for field_group - Fixed PHPUnit on GitlabCI RTBC ) had to be ported to this MR for the phpunit tests to complete, see:
https://git.drupalcode.org/project/field_group/-/merge_requests/49/diffs...
So changes to this file could probably be reverted once the phpunit tests are fixed in the main development branch (8.x-3.x).
 

We would greatly appreciate if a maintainer or someone with write permission could take a look at ticket's merge request MR!47 and let us know if there would be any more work needed.

Feel free to let us know if you have any questions or concerns on merge request MR!47 or any aspect of this ticket in general, we would surely be glad to help.
Thanks in advance for your feedback and reviews.

Production build 0.69.0 2024