Skip to content

Conversation

@BacLuc
Copy link
Contributor

@BacLuc BacLuc commented Jun 28, 2025

Exclude some controversial rules for now.

This documents the rules which were used for the last run. No enforcement right now, just enablement.

It also helped with fixing phpunit deprecations.

Exclude some controversial rules for now.

This documents the rules which were used for the last run.
No enforcement right now, just enablement.

It also helped with fixing phpunit deprecations.
@BacLuc BacLuc requested a review from a team June 28, 2025 20:37
@BacLuc
Copy link
Contributor Author

BacLuc commented Jun 28, 2025

@CodeRabbit review this pr

@coderabbitai
Copy link

coderabbitai bot commented Jun 28, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

I'll help you review this PR! Let me gather the details about the changes first.


🏁 Script executed:

#!/bin/bash
# Get PR details using GitHub CLI
gh pr view 7679 --repo ecamp/ecamp3 --json title,body,state,author,createdAt,updatedAt,additions,deletions,changedFiles,files

# Get the diff for the PR
gh pr diff 7679 --repo ecamp/ecamp3

Length of output: 178716


@BacLuc This is an excellent PR that modernizes your PHP codebase using Rector! Here's my comprehensive review:

Overall Assessment ✅

This PR demonstrates a thoughtful approach to automated refactoring. You've successfully:

  • Updated to the modern Rector configuration format
  • Applied numerous code quality improvements across 101 files
  • Been selective about which rules to apply vs. skip

Key Improvements

1. Rector Configuration Modernization

// Old format
return static function (ContainerConfigurator $containerConfigurator): void {
    // ...
};

// New format  
return RectorConfig::configure()
    ->withPaths([...])
    ->withPreparedSets(deadCode: true, codeQuality: true, ...)
    ->withSkip([...]);

The new configuration is much cleaner and more maintainable.

2. Controller Method Names

// Before
public function connectAction(Request $request, ClientRegistry $clientRegistry) {

// After  
public function connect(Request $request, ClientRegistry $clientRegistry) {

Removing the "Action" suffix follows modern Symfony conventions.

3. Code Quality Improvements

Better null checking:

// Before: isset($data->campPrototype)
// After: null !== $data->campPrototype

Simplified array operations:

// Before
$description = [];
$description['camp'] = [...];
return $description;

// After
return ['camp' => [...]];

Modern type checking:

// Before: is_a($data->category->rootContentNode, ColumnLayout::class)
// After: $data->category->rootContentNode instanceof ColumnLayout

4. PHPUnit Modernization

Data providers using generators:

public static function invalidMethods(): \Iterator {
    yield 'GET' => ['GET'];
    yield 'PUT' => ['PUT'];
    // ...
}

Better assertions:

// Before: assertEquals(12, count($data['_embedded']['contentNodes']))
// After: assertCount(12, $data['_embedded']['contentNodes'])

Smart Rule Selection

Your withSkip() configuration shows good judgment in excluding potentially controversial rules:

  • DeclareStrictTypesRector - Can be disruptive
  • FinalizeTestCaseClassRector - May conflict with inheritance patterns
  • ExplicitBoolCompareRector - Style preference

Minor Suggestions

  1. Consider enabling gradually: You might want to enable some of the skipped rules in future PRs after team discussion, like:

    • UseIdenticalOverEqualWithSameTypeRector (performance benefit)
    • SimplifyIfReturnBoolRector (readability)
  2. Documentation: Consider adding a comment in rector.php explaining why certain rules are skipped.

  3. CI Integration: If not already done, consider adding Rector to your CI pipeline for ongoing code quality maintenance.

Potential Concerns

  • Large changeset: While automated, this touches many files. Ensure thorough testing.
  • Git history: Some blame information may be lost, but the code quality gains outweigh this.

Recommendation

✅ Approve and merge - This is a well-executed modernization that significantly improves code quality while being conservative about controversial changes. The automated nature of these changes reduces the risk of introducing bugs while bringing the codebase up to modern PHP standards.

Great work on the careful rule selection and systematic approach to code modernization!

@manuelmeister manuelmeister added the deploy! Creates a feature branch deployment for this PR label Jul 22, 2025
@github-actions
Copy link

github-actions bot commented Jul 22, 2025

Feature branch deployment currently inactive.

If the PR is still open, you can add the deploy! label to this PR to trigger a feature branch deployment.

@BacLuc BacLuc added this pull request to the merge queue Jul 22, 2025
Merged via the queue into devel with commit 6f81973 Jul 22, 2025
60 checks passed
@BacLuc BacLuc deleted the readd-rector branch July 22, 2025 21:50
@BacLuc BacLuc mentioned this pull request Jul 26, 2025
This was referenced Aug 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deploy! Creates a feature branch deployment for this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants