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

Allow using NULL as the last parameter #54

Merged
merged 1 commit into from Aug 4, 2017

Conversation

Projects
None yet
2 participants
@bolknote
Contributor

bolknote commented Jul 20, 2017

Cutted example:

declare(strict_types=1);

class QueueService
{
    // ... other methods

    public function put(\Serializable $object, string $eventName, array $extraEventData, string $uniqueId = null)
    {
        return DI::gearman_client()->doBackground(
            $this->queueName,
            serialize(compact('object', 'extraEventData', 'eventName')),
            $uniqueId
        );
    }
}

$QueueServiceObject->put($foo, 'bar', $bar); // FAIL because of strict_types and doBackground requires string uniqueId
Allow using NULL as the last parameter
Cutted example:
```
declare(strict_types=1);

class QueueService
{
    // ... other methods

    public function put(\Serializable $object, string $eventName, array $extraEventData, string $uniqueId = null)
    {
        return DI::gearman_client()->doBackground(
            $this->queueName,
            serialize(compact('object', 'extraEventData', 'eventName')),
            $uniqueId
        );
    }
}

$QueueServiceObject->put($foo, 'bar', $bar); // FAIL because of strict_types and doBackground requires string uniqueId
```

@wcgallego wcgallego self-assigned this Jul 30, 2017

@wcgallego

This comment has been minimized.

Show comment
Hide comment
@wcgallego

wcgallego Jul 30, 2017

Owner

hey @bolknote, thanks for the pull request. I'll see if I can fit some time in this week to mess around with this. The change is straightforward, just want to test it a bit and see if I can add a functional test alongside

Owner

wcgallego commented Jul 30, 2017

hey @bolknote, thanks for the pull request. I'll see if I can fit some time in this week to mess around with this. The change is straightforward, just want to test it a bit and see if I can add a functional test alongside

@bolknote

This comment has been minimized.

Show comment
Hide comment
@bolknote

bolknote Jul 30, 2017

Contributor

No problem 👍

Contributor

bolknote commented Jul 30, 2017

No problem 👍

@wcgallego wcgallego merged commit 7480e92 into wcgallego:master Aug 4, 2017

@wcgallego

This comment has been minimized.

Show comment
Hide comment
@wcgallego

wcgallego Aug 4, 2017

Owner

Thanks again, @bolknote! I don't know immediately of a good way to test this (I'm pondering adding the declare strict for all tests, but that doesn't prove the opposite of this, where null is ok).

That said, this change totally makes sense! Much appreciated

Owner

wcgallego commented Aug 4, 2017

Thanks again, @bolknote! I don't know immediately of a good way to test this (I'm pondering adding the declare strict for all tests, but that doesn't prove the opposite of this, where null is ok).

That said, this change totally makes sense! Much appreciated

@bolknote

This comment has been minimized.

Show comment
Hide comment
@bolknote

bolknote Aug 5, 2017

Contributor

Welcome @wcgallego! :-) Yes I also feel this problem but I can't offer a good test.

Contributor

bolknote commented Aug 5, 2017

Welcome @wcgallego! :-) Yes I also feel this problem but I can't offer a good test.

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