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.

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

Type pinc/ProjectSearch* #1393

Merged

Conversation

jchaffraix
Copy link
Collaborator

@jchaffraix jchaffraix commented Dec 13, 2024

Typing all those files together made sense as
they are all helping the search feature.

TESTS=Run searches by different fields.

Sandbox: https://www.pgdp.org/~jchaffraix/c.branch/julien_type_projectsearch

Typing all those files together made sense as
they are all helping the search feature.

TESTS=Run searches by different fields.
@jchaffraix jchaffraix marked this pull request as ready for review December 17, 2024 07:20
@cpeel cpeel requested review from cpeel, 70ray and srjfoo December 21, 2024 18:50
Co-authored-by: Casey Peel <[email protected]>
Copy link
Member

@srjfoo srjfoo left a comment

Choose a reason for hiding this comment

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

Just a couple of questions.

@@ -83,7 +83,7 @@ class OptionSelector extends Selector
return $value;
}

public function save_value($value)
public function save_value($value): void
Copy link
Member

Choose a reason for hiding this comment

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

Does $value need to be typed?

Copy link
Collaborator 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 point. We could have typed $value as mixed to match what Settings::set_value expects. This would give us some pretty weak typing though (mixed allows anything) but better than nothing.

Since this PR was merged, I will let it be for now. We do have some higher priority sites to fix but happy to prioritize this if you really want this solved sooner rather than later.

{
$this->results_per_page = new OptionSelector(
'n_results_per_page',
_('Results per page'),
100,
'100',
Copy link
Member

Choose a reason for hiding this comment

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

int to string?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Correct, the expected type is a string so PHP was doing the conversion for us. We just made it more visible.

@cpeel cpeel merged commit ec5cd55 into DistributedProofreaders:master Jan 4, 2025
12 checks passed
Copy link
Collaborator Author

@jchaffraix jchaffraix left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, catching up after the holiday break.

@@ -83,7 +83,7 @@ class OptionSelector extends Selector
return $value;
}

public function save_value($value)
public function save_value($value): void
Copy link
Collaborator 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 point. We could have typed $value as mixed to match what Settings::set_value expects. This would give us some pretty weak typing though (mixed allows anything) but better than nothing.

Since this PR was merged, I will let it be for now. We do have some higher priority sites to fix but happy to prioritize this if you really want this solved sooner rather than later.

{
$this->results_per_page = new OptionSelector(
'n_results_per_page',
_('Results per page'),
100,
'100',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Correct, the expected type is a string so PHP was doing the conversion for us. We just made it more visible.

@jchaffraix jchaffraix deleted the julien_type_projectsearch branch January 9, 2025 06:24
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.

5 participants