Adopt strict typing

Working on it

Hi @pfrenssen

What does Update our coding standards to require strict types mean
We have to add a custom sniff for phpcs and a custom ruleset, or we just have to add a comment in the existing xml file

We should add a phpcs.xml.dist file to the root of the project.

Can you try with this as the content of the file? I didn't test it but took it from another module.

<?xml version="1.0" encoding="UTF-8"?>
<!-- https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-drupal/using-coder-and-php_codesniffer-in-gitlab-ci -->
<ruleset name="recurring_events">
  <description>PHP CodeSniffer configuration for the Recurring Events Drupal module.</description>

  <rule ref="Drupal"/>

  <arg name="extensions" value="php,inc,module,install,info,test,profile,theme,css,js"/>
  <arg name="report-width" value="120"/>

  <!-- Use 's' to print the full sniff name in the report. -->
  <arg value="s"/>

  <!-- Require strict types declaration in every PHP file. -->
  <rule ref="SlevomatCodingStandard.TypeHints.DeclareStrictTypes.DeclareStrictTypesMissing">
    <severity>5</severity>
  </rule>

</ruleset>

Hi @pfrenssen
I made the necessary changes and Included declare(strict_types=1); at the top of every PHP file.
I also Updated the coding standards to require strict types by creating a phpcs.xml.dist file
But when i am creating an MR, the pipeline is failing at composer build

This is the error that shows up:

Running with gitlab-runner 17.3.1 (66269445)
  on gitlab-runner-9dccd6855-vtfgl s8ex1X2yJ, system ID: r_Fs6duF1rd5sK
Resolving secrets
Preparing the "kubernetes" executor
00:00
"CPURequest" overwritten with "2"
Using Kubernetes namespace: gitlab-runner
Using Kubernetes executor with image drupalci/php-8.3-apache:production ...
Using attach strategy to execute scripts...
Preparing environment
00:31
Using FF_USE_POD_ACTIVE_DEADLINE_SECONDS, the Pod activeDeadlineSeconds will be set to the job timeout: 1h0m0s...
Waiting for pod gitlab-runner/runner-s8ex1x2yj-project-162884-concurrent-0-g2pu6aqs to be running, status is Pending
Running on runner-s8ex1x2yj-project-162884-concurrent-0-g2pu6aqs via gitlab-runner-9dccd6855-vtfgl...
Getting source from Git repository
00:01
Fetching changes...
Initialized empty Git repository in /builds/issue/recurring_events-3480495/.git/
Created fresh repository.
Checking out 8a88c2b2 as detached HEAD (ref is 3480495-adopt-strict-typing)...
Skipping Git submodules setup
Executing "step_script" stage of the job script
00:01
$ composer --version
Composer version 2.8.0 2024-10-02 16:40:29
PHP version 8.3.12 (/usr/local/bin/php)
Run the "diagnose" command to get more detailed diagnostics output.
$ # Forks are named "module_name-12345", so strip out the number part to get the correct module name. # collapsed multi-line command
$ if [ "$_SHOW_ENVIRONMENT_VARIABLES" == "1" ]; then # collapsed multi-line command
$ if [ "$_LENIENT_ALLOW_LIST" != "" ]; then # collapsed multi-line command
$ [[ $_CURL_TEMPLATES_REF == "" ]] && export _CURL_TEMPLATES_REF=$_GITLAB_TEMPLATES_REF # collapsed multi-line command
$ echo "Executing curl -OL https://git.drupalcode.org/$_CURL_TEMPLATES_REPO/-/raw/$_CURL_TEMPLATES_REF/scripts/expand_composer_json.php"
Executing curl -OL https://git.drupalcode.org/project/gitlab_templates/-/raw/default-ref/scripts/expand_composer_json.php
$ curl -OL https://git.drupalcode.org/$_CURL_TEMPLATES_REPO/-/raw/$_CURL_TEMPLATES_REF/scripts/expand_composer_json.php
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  8538  100  8538    0     0  69577      0 --:--:-- --:--:-- --:--:-- 69983
$ if [[ -f composer.json ]]; then # collapsed multi-line command
$ php expand_composer_json.php || EXPAND_COMPOSER_EXIT_CODE=$?
drupalConstraint = 10.3.6
core_version=10.3.6
Writing output to composer.json
$ if [[ "$EXPAND_COMPOSER_EXIT_CODE" != "" ]]; then echo "EXPAND_COMPOSER_EXIT_CODE=$EXPAND_COMPOSER_EXIT_CODE"; exit $EXPAND_COMPOSER_EXIT_CODE; fi
$ composer install $COMPOSER_EXTRA
Installing dependencies from lock file (including require-dev)
Verifying lock file contents can be installed on current platform.
Warning: The lock file is not up to date with the latest changes in composer.json. You may be getting outdated dependencies. It is recommended that you run `composer update` or `composer update <package name>`.
- Required (in require-dev) package "composer/installers" is not present in the lock file.
- Required (in require-dev) package "drupal/core-composer-scaffold" is not present in the lock file.
- Required (in require-dev) package "cweagans/composer-patches" is not present in the lock file.
- Required (in require-dev) package "drupal/core-recommended" is not present in the lock file.
- Required (in require-dev) package "drupal/core-dev" is not present in the lock file.
- Required (in require-dev) package "php-parallel-lint/php-parallel-lint" is not present in the lock file.
This usually happens when composer files are incorrectly merged or the composer.json file is manually edited.
Read more about correctly resolving merge conflicts https://getcomposer.org/doc/articles/resolving-merge-conflicts.md
and prefer using the "require" command over editing the composer.json file directly https://getcomposer.org/doc/03-cli.md#require-r
Uploading artifacts for failed job
00:02
Uploading artifacts...
.: found 373 matching artifact files and directories 
.git: excluded 1 files                             
.git/**/*: excluded 71 files                       
Uploading artifacts as "archive" to coordinator... 201 Created  id=3074846 responseStatus=201 Created token=glcbt-64
Uploading artifacts...
build.env: found 1 matching artifact files and directories 
Uploading artifacts as "dotenv" to coordinator... 201 Created  id=3074846 responseStatus=201 Created token=glcbt-64
Cleaning up project directory and file based variables
00:01
ERROR: Job failed: command terminated with exit code 1

I tried reverting the changes in composer.json and then adding back the changes and updated the lock file but the error still shows up. I also tried changing the branch in which I was merging thinking maybe i was working in the wrong branch but that didnt work as well.

Thanks for working on this! The MR has been made against the 2.x branch, but it should be done against 3.0.x instead. Can you rebase it? If the error still persists I can have a look. Normally no changes are needed in composer files.

Trying to resolve the error again

Hi @pfrenssen
I made the necessary changes rebased the commits to 3.0.x branch. the pipeline issue still shows up. Please have a look

It's because of the unrelated changes to the composer files, these files shouldn't be changed.

The composer build works again! The custom PHP CodeSniffer file also is working. It looks like 16 files are still missing the strict types declaration. And already a couple type bugs are detected in the PHPUnit tests :) That's great and the whole point of doing this.

Setting back to needs work, the remaining strict_type declarations need to be added, and the type bugs fixed so the tests are green again.

Thanks a lot for the work so far!

Thanks for the help
I'll work on the errors and solve them soon

Hey @pfrenssen
In this issue you have provided the branch recurring_events-3480495. When i ran phpcs in it, it showed a few files missing the file doc comment and only 3 files missing the strict_type declarations. I think the rest of the files that are missing the declarations are in the parent branch recurring_events.
Also, phpcs gives a recommendation to change the permission from "access administration pages" to "administer site configuration" in line 198 in the recurring_events.routing.yml file. Should I let the permission as it is, or should I change it as to what phpcs suggests.
The errors of phpunit also exist in the parent module (recurring_events) and not in the fork given in this issue.

Hey @pfrenssen

So I was trying to cast the variables into int to remove the errors in phpUnit pipeline.
Please note, that these errors are not related to the changes I made in the branch. All I changed was added strict type declarations and a custom file to add ruleset as mentioned in the issue. But when i try to solve the phpUnit pipeline error, i cast the variable given into int and then run the pipeline again and then it gives me same casting error for another variable.
Like for example - Similiarly in file MonthlyRecurringDateTest.php, the method testFindMonthDaysBetweenDates expects the $day argument to be an int, but a string is being passed. when i casted it into int and ran the pipeline again, it gave me the same error for $year, and then for $month when i ran the pipeline again and then for $timestamp. So what it is doing is giving me one error at a time and then when i solve that and commit and then push and run the pipeline again, thats when it gives me another error. I dont know how many variables need casting and at how many places as I did not write the code and dont know what has been done in each file and solving this typecasting error one by one doesnt seem the right approach.
Please have a look

This is looking very good already. Thanks also for the documentation updates.

The goal of using strict types is to find typing bugs, so these failures are showing that we have some bugs that should be fixed. There are undoubtedly more bugs, which will start to surface once this is merged. For now I will tackle the ones that are found.

Thanks @pfrenssen,

Should I move this strict type declaration issue to needs review now that the tasks mentioned in this issue are complete or let it stay in needs work?

You can always move an issue to Needs Review if you want someone to review it. But I am taking care of it now. I fixed the type bugs and am waiting for the test result. If this comes back green this is good to go and I will merge it. I'm pretty sure a bunch of typing bugs that are not yet covered by tests will surface very quickly, but these can be handled in followups.

Thanks very much for the contribution!

Tests are green! Ready to merge. Thanks!

Hi @pfrenssen
Thank you for the credit
It was my first credit. Means a lot.
I'll keep the mistakes i made in mind so that i can do even better from next time.

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