Skip to content
This repository has been archived by the owner on Jan 2, 2023. It is now read-only.

Cast getLimit() result to int instead of string #147

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Cast getLimit() result to int instead of string #147

wants to merge 1 commit into from

Conversation

FallDi
Copy link

@FallDi FallDi commented Sep 22, 2020

Currently method getLimit() returns string, which is different from PHPDoc.
It could affect PHP apps which used declare(strict_types=1);

Currently method getLimit() returns string, which is different from PHPDoc.
It could affect PHP apps which used declare(strict_types=1);
@franzliedke
Copy link
Contributor

It could affect PHP apps which used declare(strict_types=1);

As far as I know, docblocks aren't affected by strict_types, only real type hints are?

@FallDi
Copy link
Author

FallDi commented Sep 25, 2020

Yes, you're right declare(strict_types=1) affect only real type hints.

Let me show you my case.
I read PHPDoc of getLimit(), which declared return type as int.
Then I implement some code in project which relay on it

<?php declare(strict_types=1);

class OffsetLimitFilter {
	public function __construct(int $limit, int $offset) {
		// ..
	}
}

Finally I'm trying to pass getLimit() to this class

$parameters = new \Tobscure\JsonApi\Parameters($request->query->all());
$offsetOrderFilter = new OffsetLimitFilter(
    $parameters->getLimit() ? (int)$parameters->getLimit() : 10, // Cast to int is necessary due to return value actually is string, and php will throw type error
    $parameters->getOffset()
);

If this patch will be applied then source code could be simplified

$parameters = new \Tobscure\JsonApi\Parameters($request->query->all());
$offsetOrderFilter = new OffsetLimitFilter(
    $parameters->getLimit() ? : 10, // Cast to int is not necessary
    $parameters->getOffset()
);

@franzliedke
Copy link
Contributor

By all means, I like the change. Sorry for not communicating this clearly. 😆

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants