Create transport from class which don't has prefix name Elastica\Transport #1169

Merged
merged 2 commits into from Sep 6, 2016

Conversation

Projects
None yet
2 participants
@Gasol
Contributor

Gasol commented Aug 31, 2016

Allow we store configuration in JSON format, and simply restore to in-memory array and pass to Elastica\Client.

Create transport from class which don't has prefix name Elastica\Tran…
…sport

Allow we store configuration in json format, and simply restore to
in-memory array and pass to Elastica\Client.
@ruflin

This comment has been minimized.

Show comment
Hide comment
@ruflin

ruflin Aug 31, 2016

Owner

I don't fully understand this change yet. Can you share some more details and example on this?

Owner

ruflin commented Aug 31, 2016

I don't fully understand this change yet. Can you share some more details and example on this?

@Gasol

This comment has been minimized.

Show comment
Hide comment
@Gasol

Gasol Aug 31, 2016

Contributor

Assume we define configuration of Elastica in JSON format.

// config.json

{
    "servers": [
        {
            "host": "elasticsearch-1",
            "port": "9200",
            "transport": "ACME\Transport\Stream"
        }
    ],
    "timeout": "1s",
    "connectionStrategy": "RoundRobin"
}

We can simply call json_decode to get configuration in array structure, then pass it to constructor of Elastica\Client. If transport in configuration does not pre-defined in Elastica\Transport\AbstractTransport, We will get exception shows Invalid transportwithout this change.

For example:

<?php
$data = file_get_contents('/path/to/config.json');
$config = json_decode($data, true);
$client = new Elastica\Client($config);
Contributor

Gasol commented Aug 31, 2016

Assume we define configuration of Elastica in JSON format.

// config.json

{
    "servers": [
        {
            "host": "elasticsearch-1",
            "port": "9200",
            "transport": "ACME\Transport\Stream"
        }
    ],
    "timeout": "1s",
    "connectionStrategy": "RoundRobin"
}

We can simply call json_decode to get configuration in array structure, then pass it to constructor of Elastica\Client. If transport in configuration does not pre-defined in Elastica\Transport\AbstractTransport, We will get exception shows Invalid transportwithout this change.

For example:

<?php
$data = file_get_contents('/path/to/config.json');
$config = json_decode($data, true);
$client = new Elastica\Client($config);
-
- if (!class_exists($className)) {
- throw new InvalidException('Invalid transport');
+ $classNames = ["Elastica\\Transport\\$transport", $transport];

This comment has been minimized.

@ruflin

ruflin Sep 2, 2016

Owner

I'm kind of thinking if we should turn this around. Have the base assumption that $transport is the full class name, and only have as fall back the Elastica once. I would have to check where in our code we would have to adjust these paths.

@ruflin

ruflin Sep 2, 2016

Owner

I'm kind of thinking if we should turn this around. Have the base assumption that $transport is the full class name, and only have as fall back the Elastica once. I would have to check where in our code we would have to adjust these paths.

This comment has been minimized.

@Gasol

Gasol Sep 6, 2016

Contributor

Maybe we could check whether namespace separator () exists or not? It means it could be FQCN or built-in transport of Elastica right?

@Gasol

Gasol Sep 6, 2016

Contributor

Maybe we could check whether namespace separator () exists or not? It means it could be FQCN or built-in transport of Elastica right?

This comment has been minimized.

@Gasol

Gasol Sep 6, 2016

Contributor

Check namespace seperator is bad idea, We should not assume that class has a namespace.

@Gasol

Gasol Sep 6, 2016

Contributor

Check namespace seperator is bad idea, We should not assume that class has a namespace.

-
- $className = 'Elastica\\Transport\\'.$transport;
-
- if (!class_exists($className)) {

This comment has been minimized.

@ruflin

ruflin Sep 2, 2016

Owner

we should still throw an invalid exception in case none of the classes matches.

@ruflin

ruflin Sep 2, 2016

Owner

we should still throw an invalid exception in case none of the classes matches.

This comment has been minimized.

@Gasol

Gasol Sep 6, 2016

Contributor

It does, If $transport is not a AbstractTransport (Line 112), It will throw InvalidException (Line 119).

@Gasol

Gasol Sep 6, 2016

Contributor

It does, If $transport is not a AbstractTransport (Line 112), It will throw InvalidException (Line 119).

This comment has been minimized.

@ruflin

ruflin Sep 6, 2016

Owner

Got it, missed that line in the diff.

@ruflin

ruflin Sep 6, 2016

Owner

Got it, missed that line in the diff.

@ruflin

This comment has been minimized.

Show comment
Hide comment
@ruflin

ruflin Sep 2, 2016

Owner

Nice, I was never thinking of this use case. I left some minor comments in the PR. Could you also update the CHANGELOG.md?

Owner

ruflin commented Sep 2, 2016

Nice, I was never thinking of this use case. I left some minor comments in the PR. Could you also update the CHANGELOG.md?

@Gasol

This comment has been minimized.

Show comment
Hide comment
@Gasol

Gasol Sep 6, 2016

Contributor

@ruflin I have updated this PR for adding changelog.

Contributor

Gasol commented Sep 6, 2016

@ruflin I have updated this PR for adding changelog.

@ruflin ruflin merged commit 72cafba into ruflin:master Sep 6, 2016

2 checks passed

codecov/project 85.44% (+<.01%) compared to 8ce983b
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ruflin

This comment has been minimized.

Show comment
Hide comment
@ruflin

ruflin Sep 6, 2016

Owner

@Gasol Thanks, merged.

Owner

ruflin commented Sep 6, 2016

@Gasol Thanks, merged.

@Gasol Gasol deleted the Gasol:creating-transport-in-whatever-namespace branch Sep 7, 2016

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