Skip to content

Conversation

@holtkamp
Copy link
Contributor

@holtkamp holtkamp commented Jul 9, 2025

Closes #1913

@holtkamp holtkamp marked this pull request as draft July 9, 2025 18:14
@stof
Copy link
Member

stof commented Oct 16, 2025

@holtkamp given that #1923 is not settled yet, I suggest you to add the baseline entry (you can ask psalm to generate it for you by using the --set-baseline option when running psalm), so that this PR can be merged.

@holtkamp
Copy link
Contributor Author

@stof I have little experience with psalm, but gave it a try.

Using ./vendor/bin/psalm.phar resulted in:

Box Requirements Checker
========================

> Using PHP 8.4.13
> PHP is using the following php.ini file:
  /opt/homebrew/etc/php/8.4/php.ini

> Checking Box requirements:
  E............

                                                                                                                                                                                                                                                                 
 [ERROR] Your system is not ready to run the application.                                                                                                                                                                                                        
                                                                                                                                                                                                                                                                 

Fix the following mandatory requirements:
=========================================

 * The application requires the version "^7.4 || ~8.0.0 || ~8.1.0 || ~8.2.0 || ~8.3.0" or greater.

After that:

./vendor/bin/psalm.phar --debug                         
PHP Deprecated:  Constant E_STRICT is deprecated in phar:///Users/workspace/aws-holtkamp/vendor/psalm/phar/psalm.phar/src/Psalm/Internal/ErrorHandler.php on line 54

Deprecated: Constant E_STRICT is deprecated in phar:///Users/workspace/aws-holtkamp/vendor/psalm/phar/psalm.phar/src/Psalm/Internal/ErrorHandler.php on line 54

So I installed psalm globally, which seem to work, but gave a lot of validation messages:

2012 errors found
------------------------------
5496 other issues found.
You can display them with --show-info=true
------------------------------
Psalm can automatically fix 1282 of these issues.
Run Psalm again with 
--alter --issues=MissingOverrideAttribute,InvalidFalsableReturnType,InvalidReturnType,InvalidNullableReturnType,ClassMustBeFinal,MissingParamType --dry-run
to see what it can fix.
----------------------------

and a huge baseline, which is incorrect / not preferred I suppose.

I think the quickest way to resolve this is that you generate the baseline? Also feel free to amend the PR.

@stof
Copy link
Member

stof commented Oct 23, 2025

If you use a different version of Psalm than what our CI uses, getting different list of errors is quite expected, and it will break the CI checks (which will still run the version we use).

unfortunately, our CI setup uses an old version of psalm.

@holtkamp
Copy link
Contributor Author

@stof ok, what I did:

  • synced the branch
  • downgraded to PHP 8.3
  • removed the locally installed Psalm
  • executed composer update
  • executed ./vendor/bin/psalm.phar --set-baseline

However, nothing changed to the baseline...

Complete output:

Target PHP version: 8.3 (inferred from current PHP version) Enabled extensions: dom, simplexml.
Scanning files...
Analyzing files...



ERROR: MoreSpecificReturnType - src/Service/CloudWatchLogs/src/Result/DescribeLogGroupsResponse.php:100:16 - The declared return type 'list<'ACCOUNT_DATA_PROTECTION'>' for AsyncAws\CloudWatchLogs\Result\DescribeLogGroupsResponse::populateResultInheritedProperties is more specific than the inferred return type 'list<string>' (see https://psalm.dev/070)
     * @return list<InheritedProperty::*>


ERROR: LessSpecificReturnStatement - src/Service/CloudWatchLogs/src/Result/DescribeLogGroupsResponse.php:112:16 - The type 'list<string>' is more general than the declared return type 'list<'ACCOUNT_DATA_PROTECTION'>' for AsyncAws\CloudWatchLogs\Result\DescribeLogGroupsResponse::populateResultInheritedProperties (see https://psalm.dev/129)
        return $items;


------------------------------
2 errors found
------------------------------
6656 other issues found.
You can display them with --show-info=true
------------------------------

Checks took 0.47 seconds and used 103.644MB of memory
No files analyzed
Psalm was able to infer types for 95.7931% of the codebase

So I added the lines that were suggested by @jderusse manually to psalm-baseline.xml:

 <file src="src/Service/CloudWatchLogs/src/Result/DescribeLogGroupsResponse.php">
   <LessSpecificReturnStatement>
     <code><![CDATA[$items]]></code>
   </LessSpecificReturnStatement>
   <MoreSpecificReturnType>
     <code><![CDATA[list<InheritedProperty::*>]]></code>
   </MoreSpecificReturnType>
 </file>

after which Psalm did not report errors anymore:

./vendor/bin/psalm.phar               
Target PHP version: 8.3 (inferred from current PHP version) Enabled extensions: dom, simplexml.
Scanning files...
Analyzing files...



------------------------------
                              
       No errors found!       
                              
------------------------------
6658 other issues found.
You can display them with --show-info=true
------------------------------

Checks took 0.45 seconds and used 103.569MB of memory
No files analyzed
Psalm was able to infer types for 95.7931% of the codebase

Hopefully this suffices...

@stof
Copy link
Member

stof commented Oct 27, 2025

@holtkamp with this old version of Psalm, I actually had to use ./vendor/bin/psalm.phar --update-baseline --set-baseline=psalm.baseline.xml for it to work.
Thankfully, we will soon use a more uptodate Psalm version: #1973

@stof
Copy link
Member

stof commented Oct 27, 2025

Make sure you rerun the code generator as your generated code is outdated.

@holtkamp
Copy link
Contributor Author

@stof ok, also executed ./generate CloudWatchLogs which resulted in:

 Select the operation to generate:
  [0] <all operation>
  [1] <new operation>
  [2] CreateLogGroup
  [3] CreateLogStream
  [4] DescribeLogGroups
  [5] DescribeLogStreams
  [6] FilterLogEvents
  [7] PutLogEvents
 > 0
                                                                  
 [OK] Operations generated

@stof stof marked this pull request as ready for review October 27, 2025 10:18
@stof
Copy link
Member

stof commented Oct 27, 2025

@holtkamp Please also add Added `DescribeLogGroups` method in the Added section of the changelog (in the Not released section)

@stof stof merged commit 9005112 into async-aws:master Oct 27, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CloudWatchLogs] CloudWatchLogsClient::describeLogGroups() not implemented?

3 participants