Skip to content
This repository was archived by the owner on Jan 8, 2020. It is now read-only.

Fixed ZF2-100: Use new naming convention for zend config options#588

Closed
proofek wants to merge 1 commit into
zendframework:masterfrom
proofek:hotfix/ZF2-100
Closed

Fixed ZF2-100: Use new naming convention for zend config options#588
proofek wants to merge 1 commit into
zendframework:masterfrom
proofek:hotfix/ZF2-100

Conversation

@proofek

@proofek proofek commented Nov 11, 2011

Copy link
Copy Markdown

No description provided.

@b-durand

Copy link
Copy Markdown
Contributor

I disagree. Where come from this new naming convention?

@proofek

proofek commented Nov 11, 2011

Copy link
Copy Markdown
Author

See http://framework.zend.com/issues/browse/ZF2-100 for reference.

@b-durand

Copy link
Copy Markdown
Contributor

When we say don't use underscore, we means $_foo not 'foobar' instead of 'foo_bar'. If we talk about ZendMonitor, you write this 'zend_monitor' and not 'zendmonitor'.

@sasezaki

Copy link
Copy Markdown
Contributor

FYI.
current head's adapter has "headers key(ZF1 strtolower --> ZF2 ucrfirst-uchrfirst)" bug
http://framework.zend.com/issues/browse/ZF2-99

Socket Adapter's fix is
https://gist.github.com/1358388

If you run test tests/Zend/Http/Client/Adapter/*, "headers key" bug will block tests .

(I would pull-request for ZF2-99 fix later )

@weierophinney

Copy link
Copy Markdown
Member

Closing -- intiilapa's comments hit the nail on the head. We're removing underscore prefixes for non-public variables and method names, but encouraging underscore separators for config options.

@proofek

proofek commented Nov 11, 2011

Copy link
Copy Markdown
Author

Well in that case library/Zend/Http/Client.php is broken, as it removes all underscores in config.

public function setConfig($config = array())
{
    if ($config instanceof Config) {
        $config = $config->toArray();

    } elseif (!is_array($config)) {
        throw new Exception\InvalidArgumentException('Config parameter is not valid');
    }

    /** Config Key Normalization */
    foreach ($config as $k => $v) {
        $this->config[str_replace(array('-', '_', ' ', '.'), '', strtolower($k))] = $v; // replace w/ normalized
    }

    // Pass configuration options to the adapter if it exists
    if ($this->adapter instanceof Client\Adapter) {
        $this->adapter->setConfig($config);
    }

    return $this;
}

@ralphschindler

Copy link
Copy Markdown
Member

First, after the Options proposal is ratified, this will be a non-issue.

But secondly, more importantly, normalization allows for both 'keep_alive' and 'keepalive' to both be passed as a config option and all the subsequent classes the ability to read the value for that config value.

Also, I don't think this has anything to do with the @ZF2-99 that was reported, that seems like a case of keepalive being passed in, but also not waiting for the socket to finish what it is doing.

@proofek

proofek commented Nov 28, 2011

Copy link
Copy Markdown
Author

Well, if you gonna go for the support of both conventions then Proxy class needs to be changed to accomodate that. At the moment it's broken and will not proxy requests properly. Do you reckon it should check for both proxy_host and proxyhost options then?

@tcz

tcz commented Feb 20, 2012

Copy link
Copy Markdown

I think an Adapter abstract should do the normalization in all cases. However, this is broken, so I think temporarily accepting the pull request above would be fine.

I came here after an hour debugging Mink+Goutte, which uses the entire ZF2 in order to function for some unclear reason.

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.

6 participants