Problem/Motivation

The new Gitlab CI pipeline reports a bunch of eslint/phpcs/phpstan issues. We should fix these to make the pipeline completely green.

Proposed resolution

Fix all code style related issues.

Issue fork mailchimp_transactional-3422332

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

DieterHolvoet created an issue. See original summary.

Anjali Mehta made their first commit to this issue’s fork.

Anjali Mehta opened merge request !26

zaryab_drupal made their first commit to this issue’s fork.

utkarsh_kumar_singh’s picture

utkarsh_kumar_singh commented
Assigned: Unassigned » utkarsh_kumar_singh

I am working on it.

dieterholvoet changed the visibility of the branch 1.0.x to hidden.

dieterholvoet changed the visibility of the branch 3422332-1.0.x to hidden.

utkarsh_kumar_singh’s picture

utkarsh_kumar_singh commented
Assigned: utkarsh_kumar_singh » Unassigned
Status: Active » Needs review

Fix the eslint warnings and fix some phpstan errors.

dieterholvoet’s picture

dieterholvoet commented
Status: Needs review » Needs work

The pipeline is not passing yet.

tom konda made their first commit to this issue’s fork.

ankitv18 made their first commit to this issue’s fork.

ankitv18 changed the visibility of the branch 3422332-fix-gitlab-pipeline to hidden.

danrod’s picture

danrod
Location Ottawa
commented
Assigned: Unassigned » danrod
danrod’s picture

danrod
Location Ottawa
commented
Assigned: danrod » Unassigned
danrod’s picture

danrod
Location Ottawa
commented

I fixed all of the ESLINT / PHPUnit tests issues and it's all green now: https://git.drupalcode.org/project/mailchimp_transactional/-/merge_reque...

I had to refactor the MailchimpTransactionalPluginMailTest.php file to make the test to pass, anyways, the tests need some refactoring, I'm not sure why $this->config /code>is <code>NULL in the TransactionMail class when running the PHPUnit tests.

danrod’s picture

danrod
Location Ottawa
commented
Status: Needs work » Needs review
dieterholvoet’s picture

dieterholvoet commented
Status: Needs review » Needs work

We'll have to drop support for PHP 7.3 if we're adding property types, but I think that's acceptable. Left some comments in the MR.

danrod’s picture

danrod
Location Ottawa
commented
Assigned: Unassigned » danrod
danrod’s picture

danrod
Location Ottawa
commented
Assigned: danrod » Unassigned
danrod’s picture

danrod
Location Ottawa
commented

I fixed all issues mentioned in the comments above, except for the Mail test of course

https://git.drupalcode.org/issue/mailchimp_transactional-3422332/-/jobs/...

1) MailchimpTransactionalPluginMailTest::testMailPluginMailFunction
Tested sending message to multiple recipients.
Failed asserting that false is true.
/builds/issue/mailchimp_transactional-3422332/tests/src/Functional/MailchimpTransactionalPluginMailTest.php:32
FAILURES!
Tests: 8, Assertions: 23, Failures: 1.

I can't think of a solution at the moment, will revisit this later shortly I hope.

dieterholvoet’s picture

dieterholvoet commented
Status: Needs work » Needs review

Test is fixed.

ankitv18’s picture

ankitv18 commented

Changes looks good ~~ Pipeline are passing , CSpell is still having some smell which I can guess cover in this only.
Keeping this in NR for other to take a look.

dieterholvoet’s picture

dieterholvoet commented
Related issues: +#3543322: Clean up config keys

I created a separate issue for the remaining CSpell issue: #3543322: Clean up config keys. I'm going to apply a patch of these changes to a website that uses the module a lot and keep it running for a couple weeks. If all is good afterwards, I'll merge this.

dieterholvoet’s picture

dieterholvoet commented
Status: Needs review » Fixed

Tests are finally passing again. Thanks for the work everyone!

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.

danrod’s picture

danrod
Location Ottawa
commented

Thanks and don't forget to add the credits to anyone who contributed on this, if possible @dieterholvoet .

dieterholvoet’s picture

dieterholvoet commented

I already tried a couple times but the new contribution record system has been acting up. I tried again.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.