Skip to content

Conversation

@tacsipacsi
Copy link
Contributor

  • Add missing return types. In closures touched anyway, also add parameter types.
  • Avoid capturing unused variables. They are used in the commented-out part; to make the commented-out part work out of the box if uncommented, use arrow functions (available since PHP 7.4) instead of traditional anonymous functions: arrow functions capture variables automatically, so the use block can simply be skipped. Use arrow functions for other touched closures as well.
  • Add return types to JsonSerializable::jsonSerialize() overrides. The lack of return types cause deprecation warnings on PHP 8. The recommended return type of mixed cannot be used on PHP 7.4 (which we still support according to composer.json), but fortunately all classes consistently return arrays, so array could be added as a return type instead (which is available in PHP 7.4 as well, and which is stricter than the type stated by the interface – covariant return types are allowed since PHP 7.4).

I skipped a few errors that look real and that I don’t know how to fix.

@tacsipacsi
Copy link
Contributor Author

By the way, does this project really still support PHP 7.4? If we’d drop PHP 7.4 support, maybe even #59 could be merged some day (which is basically stalled on too old PHP versions pinned in composer.json).

@jbelien
Copy link
Member

jbelien commented Sep 25, 2025

Thanks A LOT ! ☺️

- Add missing return types. In closures touched anyway, also add
  parameter types.
- Avoid capturing unused variables. They are used in the commented-out
  part; to make the commented-out part work out of the box if
  uncommented, use arrow functions (available since PHP 7.4) instead of
  traditional anonymous functions: arrow functions capture variables
  automatically, so the `use` block can simply be skipped. Use arrow
  functions for other touched closures as well.
- Add return types to `JsonSerializable::jsonSerialize()` overrides. The
  lack of return types cause deprecation warnings on PHP 8. The
  recommended return type of `mixed` cannot be used on PHP 7.4 (which we
  still support according to `composer.json`), but fortunately all
  classes consistently return arrays, so `array` could be added as a
  return type instead (which is available in PHP 7.4 as well, and which
  is stricter than the type stated by the interface – covariant return
  types are allowed since PHP 7.4).
- Fix a wrongly documented type: the nicknames are objects, not strings.
  Refactor the code a bit to make it easier to tell this PHPStan.

I skipped one error that looks real and that I don’t know how to fix:
the documentation states that `App\Model\Overpass\Element::$tags` is
nullable but it’s used as if it wasn’t; I don’t know Overpass, so I have
no idea which one is right.
@jbelien
Copy link
Member

jbelien commented Sep 26, 2025

Just need to rebase your PR 👌

@jbelien jbelien merged commit ecab9e0 into EqualStreetNames:master Oct 4, 2025
3 of 4 checks passed
@tacsipacsi
Copy link
Contributor Author

Thanks for rebasing and merging! Sorry, I’ve been a bit busy IRL in the past week.

@tacsipacsi tacsipacsi deleted the phpstan branch October 5, 2025 09:45
@tacsipacsi
Copy link
Contributor Author

Also, could you please take care of the remaining two errors? It’s important that there are no permanent errors, as it makes new ones much easier to spot.

@jbelien
Copy link
Member

jbelien commented Oct 5, 2025

See #73

@tacsipacsi
Copy link
Contributor Author

Thanks, but I asked you to take care of them because I think the errors should be addressed rather than suppressed. I could also have added the @phpstan-ignore comments, but adding the comment means the problem is still there.

  • Either the type declaration is wrong, and App\Model\Overpass\Element::$tags should be documented to be object rather than ?object;
  • or the usage is wrong, and the code should check if $object->tags is not null, otherwise we risk PHP notices (PHP 7.x) / warnings (PHP 8.x).

(On the other hand, passing CI is still better than failing CI, so thanks anyway.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants