Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enable declare_strict_types option and apply @PHP80Migration set with… #2190

Merged
merged 8 commits into from
Mar 25, 2024
Merged

Enable declare_strict_types option and apply @PHP80Migration set with… #2190

merged 8 commits into from
Mar 25, 2024

Conversation

sidz
Copy link
Contributor

@sidz sidz commented Jan 27, 2024

This PR enables declare_strict_types option and applies @PHP80Migration set with risky rules.

ruflin
ruflin reviewed Jan 29, 2024
View reviewed changes
Copy link
Owner

@ruflin ruflin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this triggered quite a list of CI failures :-)

src/AbstractUpdateAction.php
@@ -1,5 +1,7 @@
<?php

declare(strict_types=1);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this line automatically added to each file or is this something done manually?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the help of PHP-CS-Fixer declare_strict_types rule included in @PHP80Migration:risky rule set.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, I was worried there would a be manual step involved.

@sidz
Copy link
Contributor Author

sidz commented Jan 29, 2024

Looks like this triggered quite a list of CI failures :-)

Yeah. Here is a lot what need to be done.

@sidz sidz marked this pull request as draft March 24, 2024 12:36
@sidz sidz force-pushed the enable-strict-types branch 4 times, most recently from 84f70e5 to 26246cb Compare March 24, 2024 16:35
@sidz sidz marked this pull request as ready for review March 24, 2024 16:49
@sidz
Copy link
Contributor Author

sidz commented Mar 24, 2024 •
edited
Loading

ready for review @ruflin

ruflin
ruflin approved these changes Mar 25, 2024
View reviewed changes
Copy link
Owner

@ruflin ruflin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change LGTM. I'll get this in directly to make sure we don't have any future conflicts. I have one question around a change in the test but we can still follow up on this separately.

tests/Cluster/SettingsTest.php
@@ -38,7 +40,7 @@ public function testSetReadOnly(): void
$index->addDocument($doc2);
$this->fail('should throw read only exception');
} catch (ClientResponseException $e) {
$error = \json_decode($e->getResponse()->getBody(), true)['error'] ?? null;
$error = \json_decode((string) $e->getResponse()->getBody(), true)['error']['root_cause'][0] ?? null;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know why this changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's a good catch:
Seems like elasticsearch response looks like:

{
  "error": {
    "type": "cluster_block_exception",
    "root_cause": [
      {
        "type": "cluster_block_exception",
        ...
      }
    ]
  }
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why the test passed before 🤔 If I remember correctly, there was a major ES version where the response format changed, but that was quite a while ago. Good to know we have it covered in the tests.

tests/Index/SettingsTest.php
@@ -136,8 +137,7 @@ public function testDeleteAliasWithException(): void
$indexAlias->delete();
$this->fail('Should throw exception because you should delete the concrete index and not the alias');
} catch (ClientResponseException $e) {
$response = ResponseConverter::toElastica($e->getResponse());
$error = $response->getFullError();
$error = \json_decode((string) $e->getResponse()->getBody(), true)['error']['root_cause'][0] ?? null;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason you removed the ResponseCoverter here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ResponseCoverter in my opinion should work with code related to src folder (production code). Official Elasticsearch client does not return ResponseInterface.

I'm planning to raise a new PR where we won't allow ResponseInterface as an option to ResponseCoverter

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this make migration from 7.x potentially harder? Lets discuss on the upcoming PR, code will make it much easier to fully understand what you mean by this.

tests/Query/CombinedFieldsQueryTest.php Show resolved Hide resolved
@ruflin ruflin merged commit cbc602a into ruflin:8.x Mar 25, 2024
17 of 19 checks passed
@ruflin
Copy link
Owner

ruflin commented Mar 25, 2024

Thanks for all the cleanup @sidz One thing I just noticed after merging, no changelog 🤦

@sidz sidz deleted the enable-strict-types branch March 25, 2024 07:09
@sidz
Copy link
Contributor Author

sidz commented Mar 25, 2024

Thanks for all the cleanup @sidz One thing I just noticed after merging, no changelog 🤦

I'll add in the separate PR

@sidz sidz mentioned this pull request Mar 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ruflin ruflin ruflin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sidz @ruflin