Added autoParagraph functionality to TextHelper #1025

Closed
wants to merge 443 commits into
from

Conversation

Projects
None yet
@TeckniX
Contributor

TeckniX commented Dec 14, 2012

There's more than my file on this pull-request, which I'm not sure I understand.
Following previous autoP discussion, this is an update to the function in the TextHelper only, and renamed as autoParagraph.

I haven't done 'tests' in cake before, so I will not be able to create the proper test cases. I apologize for my lack of knowledge on this area of CakePHP

dogmatic69 and others added some commits Sep 14, 2012

Adding a check to the object before use
For some reason that I have not been able to figure out yet the object is
returned as null.  This is causing some exceptions when trying to access
properties that dont exist.

FatalErrorException: "Call to a member function getAssociated() on a non-object"

Test still pass with the added check.
Merge pull request #844 from dogmatic69/form-bug
Adding a check to the object before use
Merge pull request #845 from dogmatic69/security-component-cleanup
You cant pass func_get_args() in PHP < 5.3
Cleaning up the RequestHandlerCompoent
removing redundant code and shifting a few things around so there
are less nested ifs and making things easier to follow.

Removing some variable setting, returning function calls instead.
Merge pull request #839 from dogmatic69/cleanup-request-handler
Cleaning up the RequestHandlerCompoent
Merge pull request #843 from dereuromark/2.3-ticket-3172
check() for CookieComponent and Configure
Merge pull request #849 from dogmatic69/rijndael-fatal-error
skipping the rijndael test if mcrypt_encrypt() is not available
Implement resetSequence()
This is an optional method in DboSource that allows datasources to
resetSequence values in tables.  This is useful for datasources like
Postgres, and makes using fixtures much easier.

Fixes #3026
Merge pull request #850 from ceeram/2.2-filesize
Add CakeNumber::fromReadableSize() and Validation::filesize()
Merge pull request #835 from tigrang/default-block-value
Added ability to set default view block content
Merge pull request #856 from ceeram/2.3-cakenumber
Simplify and improve fromReadableSize().

lorenzo and others added some commits Dec 1, 2012

Merge pull request #998 from dereuromark/2.3-bake-fixture-fix
Remove double semicolon in generated fixtures.

Fixes #3429
simplify some model code
remove nested if's and else where possible and early return.
Merge pull request #1002 from dereuromark/2.3-new-lines
unify new lines at the end of the file
Merge pull request #1004 from dogmatic69/hash-expand-integer-key
Fix issue where Hash::expand() sets incorrect array keys

Fixes #3434
Merge branch 'master' into 2.3
Conflicts:
	lib/Cake/VERSION.txt
Merge pull request #1000 from dogmatic69/validation-simplify
Validation simplify

Simplify internals in Model/Validation
@ceeram

This comment has been minimized.

Show comment Hide comment
@ceeram

ceeram Dec 10, 2012

Member

cheating? or forgot to remove?

cheating? or forgot to remove?

This comment has been minimized.

Show comment Hide comment
@ceeram

ceeram Dec 10, 2012

Member

Fixed here: a8eca60#L2L133

Member

ceeram replied Dec 10, 2012

Fixed here: a8eca60#L2L133

This comment has been minimized.

Show comment Hide comment
@markstory

markstory Dec 10, 2012

Member

I'm stupid, that's the problem there. Thanks for fixing it.

Member

markstory replied Dec 10, 2012

I'm stupid, that's the problem there. Thanks for fixing it.

ceeram and others added some commits Dec 10, 2012

Merge pull request #1014 from ceeram/phpcs
ignore last few silencers for code sniffer
Merge branch 'master' into 2.3
Conflicts:
	lib/Cake/Model/CakeSchema.php
	lib/Cake/Test/Case/Model/Datasource/Database/MysqlTest.php
Added autoParagraph functionality
Following previous autoP discussion, this is an update to the function. I haven't done 'test' in cake before, so I will not be able to create the proper test cases.
New lines need to be preserved
Simple line returns still need to look proper - nl2br added
@markstory

This comment has been minimized.

Show comment Hide comment
@markstory

markstory Dec 14, 2012

Member

Looks like your merge is mixed up. I think you want this to target 2.3 instead of master.

Member

markstory commented Dec 14, 2012

Looks like your merge is mixed up. I think you want this to target 2.3 instead of master.

@markstory markstory closed this Dec 14, 2012

@TeckniX

This comment has been minimized.

Show comment Hide comment
@TeckniX

TeckniX Dec 14, 2012

Contributor

I went to edit only 1 file from the 2.3 branch - Not sure how it created the rest?

Contributor

TeckniX commented Dec 14, 2012

I went to edit only 1 file from the 2.3 branch - Not sure how it created the rest?

@ADmad

This comment has been minimized.

Show comment Hide comment
@ADmad

ADmad Dec 14, 2012

Member

@TeckniX The problem is your PR is targeted to the master branch instead of 2.3. When creating a PR change the "base branch" to 2.3

Member

ADmad commented Dec 14, 2012

@TeckniX The problem is your PR is targeted to the master branch instead of 2.3. When creating a PR change the "base branch" to 2.3

@TeckniX

This comment has been minimized.

Show comment Hide comment
@TeckniX

TeckniX Dec 14, 2012

Contributor

Figured it out - thanks!

Contributor

TeckniX commented Dec 14, 2012

Figured it out - thanks!

@ryantology

This comment has been minimized.

Show comment Hide comment
@ryantology

ryantology Jan 18, 2013

It looks like this change is throwing new warnings. I am still trying to track down a proper abstracted test case but it looks like the change is not compatible with ElasticSource https://github.com/dkullmann/CakePHP-Elastic-Search-DataSource The new 'context' parameter is throwing a notice. I am going to look into how the Elastic Datasource is generating URLs and see if this can be updated.

This is the debug output that I have so far.


Notice (8): Array to string conversion [CORE/Cake/Network/Http/HttpSocket.php, line 718]
Code Context
$uri = array(
    'host' => '*****',
    'port' => '*****',
    'scheme' => 'http',
    'user' => null,
    'pass' => null,
    'path' => 'content/item/_search',
    'query' => '',
    'fragment' => null,
    'persistent' => false,
    'protocol' => (int) 6,
    'timeout' => (int) 30,
    'context' => array(
        'ssl' => array(
            'verify_peer' => true,
            'verify_depth' => (int) 5,
            'cafile' => '/Users/ryanw/Documents/Projects/PROJECT/lib/Cake/Config/cacert.pem',
            'CN_match' => '127.0.0.1'
        )
    )
)
$uriTemplate = 'http://127.0.0.1:9200/content/item/_search'
$stripIfEmpty = array(
    'host' => '*****',
    'query' => '?%query',
    'fragment' => '#%fragment',
    'user' => '%user:%pass@'
)
$key = 'host'
$strip = '%host:%port/'
$defaultPorts = array(
    'http' => (int) 80,
    'https' => (int) 443
)
$property = 'context'
$value = array(
    'ssl' => array(
        'verify_peer' => true,
        'verify_depth' => (int) 5,
        'cafile' => '/Users/ryanw/Documents/Projects/PROJECT/lib/Cake/Config/cacert.pem',
        'CN_match' => '127.0.0.1'
    )
)
str_replace - [internal], line ??
HttpSocket::_buildUri() - CORE/Cake/Network/Http/HttpSocket.php, line 718
HttpSocket::url() - CORE/Cake/Network/Http/HttpSocket.php, line 567
ElasticSource::logQuery() - APP/Plugin/Elastic/Model/Datasource/ElasticSource.php, line 1607
ElasticSource::execute() - APP/Plugin/Elastic/Model/Datasource/ElasticSource.php, line 1417
ElasticSource::__call() - APP/Plugin/Elastic/Model/Datasource/ElasticSource.php, line 1378
ElasticSource::get() - APP/Plugin/Elastic/Model/Datasource/ElasticSource.php, line 244
ElasticSource::read() - APP/Plugin/Elastic/Model/Datasource/ElasticSource.php, line 244
Model::find() - CORE/Cake/Model/Model.php, line 2674
ContentitemElastic::getRecent() - APP/Model/ContentitemElastic.php, line 54
ReflectionMethod::invokeArgs() - [internal], line ??
Controller::invokeAction() - CORE/Cake/Controller/Controller.php, line 485
Dispatcher::_invoke() - CORE/Cake/Routing/Dispatcher.php, line 186
Dispatcher::dispatch() - CORE/Cake/Routing/Dispatcher.php, line 161
[main] - APP/webroot/index.php, line 92

It looks like this change is throwing new warnings. I am still trying to track down a proper abstracted test case but it looks like the change is not compatible with ElasticSource https://github.com/dkullmann/CakePHP-Elastic-Search-DataSource The new 'context' parameter is throwing a notice. I am going to look into how the Elastic Datasource is generating URLs and see if this can be updated.

This is the debug output that I have so far.


Notice (8): Array to string conversion [CORE/Cake/Network/Http/HttpSocket.php, line 718]
Code Context
$uri = array(
    'host' => '*****',
    'port' => '*****',
    'scheme' => 'http',
    'user' => null,
    'pass' => null,
    'path' => 'content/item/_search',
    'query' => '',
    'fragment' => null,
    'persistent' => false,
    'protocol' => (int) 6,
    'timeout' => (int) 30,
    'context' => array(
        'ssl' => array(
            'verify_peer' => true,
            'verify_depth' => (int) 5,
            'cafile' => '/Users/ryanw/Documents/Projects/PROJECT/lib/Cake/Config/cacert.pem',
            'CN_match' => '127.0.0.1'
        )
    )
)
$uriTemplate = 'http://127.0.0.1:9200/content/item/_search'
$stripIfEmpty = array(
    'host' => '*****',
    'query' => '?%query',
    'fragment' => '#%fragment',
    'user' => '%user:%pass@'
)
$key = 'host'
$strip = '%host:%port/'
$defaultPorts = array(
    'http' => (int) 80,
    'https' => (int) 443
)
$property = 'context'
$value = array(
    'ssl' => array(
        'verify_peer' => true,
        'verify_depth' => (int) 5,
        'cafile' => '/Users/ryanw/Documents/Projects/PROJECT/lib/Cake/Config/cacert.pem',
        'CN_match' => '127.0.0.1'
    )
)
str_replace - [internal], line ??
HttpSocket::_buildUri() - CORE/Cake/Network/Http/HttpSocket.php, line 718
HttpSocket::url() - CORE/Cake/Network/Http/HttpSocket.php, line 567
ElasticSource::logQuery() - APP/Plugin/Elastic/Model/Datasource/ElasticSource.php, line 1607
ElasticSource::execute() - APP/Plugin/Elastic/Model/Datasource/ElasticSource.php, line 1417
ElasticSource::__call() - APP/Plugin/Elastic/Model/Datasource/ElasticSource.php, line 1378
ElasticSource::get() - APP/Plugin/Elastic/Model/Datasource/ElasticSource.php, line 244
ElasticSource::read() - APP/Plugin/Elastic/Model/Datasource/ElasticSource.php, line 244
Model::find() - CORE/Cake/Model/Model.php, line 2674
ContentitemElastic::getRecent() - APP/Model/ContentitemElastic.php, line 54
ReflectionMethod::invokeArgs() - [internal], line ??
Controller::invokeAction() - CORE/Cake/Controller/Controller.php, line 485
Dispatcher::_invoke() - CORE/Cake/Routing/Dispatcher.php, line 186
Dispatcher::dispatch() - CORE/Cake/Routing/Dispatcher.php, line 161
[main] - APP/webroot/index.php, line 92

This comment has been minimized.

Show comment Hide comment
@ryantology

ryantology Jan 18, 2013

The issue has to do with the persistent HttpSocket instance. It looks like the SSL peer verification does not clean up the 'context' value between requests. Here is an example.

    public function example() {
        App::uses('HttpSocket', 'Network/Http');
        $config = array(
            'datasource' => 'Example',
            'host' => '127.0.0.1',
            'port' => 80,
            'format' => 'application/json',
            'encoding' => 'utf-8',
        );
        extract($config);
        $scheme = 'http';
        $request = array('uri' => compact('host', 'port', 'scheme'), 'header' => array('Accept' => $config['format'], 'Content-Type' => $config['format']));
        $httpConfig = compact('host', 'port', 'request');
        $this->Http = new HttpSocket($httpConfig);

        // string query     
        $path = '/content/';
        $query  = null;
        $uri = $this->_uri(compact('path', 'query'));
        $body = null;
        debug($uri);
        $response = $this->Http->get($uri, array(), compact('body'));
        $uri = $this->_uri(compact('path', 'query'));
        debug($uri);
        $response = $this->Http->get($uri, array(), compact('body'));
        debug($response);

        $this->autoRender = false;
    }

    protected function _uri($config) {
        $config = Set::merge($this->Http->config, $config);
        unset($config['request']);
        return $config;
    }

The issue has to do with the persistent HttpSocket instance. It looks like the SSL peer verification does not clean up the 'context' value between requests. Here is an example.

    public function example() {
        App::uses('HttpSocket', 'Network/Http');
        $config = array(
            'datasource' => 'Example',
            'host' => '127.0.0.1',
            'port' => 80,
            'format' => 'application/json',
            'encoding' => 'utf-8',
        );
        extract($config);
        $scheme = 'http';
        $request = array('uri' => compact('host', 'port', 'scheme'), 'header' => array('Accept' => $config['format'], 'Content-Type' => $config['format']));
        $httpConfig = compact('host', 'port', 'request');
        $this->Http = new HttpSocket($httpConfig);

        // string query     
        $path = '/content/';
        $query  = null;
        $uri = $this->_uri(compact('path', 'query'));
        $body = null;
        debug($uri);
        $response = $this->Http->get($uri, array(), compact('body'));
        $uri = $this->_uri(compact('path', 'query'));
        debug($uri);
        $response = $this->Http->get($uri, array(), compact('body'));
        debug($response);

        $this->autoRender = false;
    }

    protected function _uri($config) {
        $config = Set::merge($this->Http->config, $config);
        unset($config['request']);
        return $config;
    }

This comment has been minimized.

Show comment Hide comment
@markstory

markstory Jan 19, 2013

Member

I'm a bit confused as I don't use this specific datasource. Is it just merging all the config options into the request array? I don't see where in HttpSocket the context is merged into the request data. Also your example doesn't use persistent connections, is it still related to that?

Member

markstory replied Jan 19, 2013

I'm a bit confused as I don't use this specific datasource. Is it just merging all the config options into the request array? I don't see where in HttpSocket the context is merged into the request data. Also your example doesn't use persistent connections, is it still related to that?

This comment has been minimized.

Show comment Hide comment
@ryantology

ryantology Jan 19, 2013

Sorry, the point about persistence wasn't very clear. What I meant was that the instance of Httpsocket is re-used rather than re-instantiated between requests.

As you pointed out the issue has to do with the reuse of $this->Http->config as the merged array of the uri value for Httpsocket::get The change in SSL peer verification commit pollutes the Httpsocket::config array and does not clean up the context value between requests.

It could be argued that ElasticSource is not implementing Httpsocket properly by merging the config parameters. As a patch to Elastic source I will remove the context parameter from the uri generator but it is possible that other people are implementing this the same way.

Sorry, the point about persistence wasn't very clear. What I meant was that the instance of Httpsocket is re-used rather than re-instantiated between requests.

As you pointed out the issue has to do with the reuse of $this->Http->config as the merged array of the uri value for Httpsocket::get The change in SSL peer verification commit pollutes the Httpsocket::config array and does not clean up the context value between requests.

It could be argued that ElasticSource is not implementing Httpsocket properly by merging the config parameters. As a patch to Elastic source I will remove the context parameter from the uri generator but it is possible that other people are implementing this the same way.

This comment has been minimized.

Show comment Hide comment
@markstory

markstory Jan 19, 2013

Member

I'll see if I can reproduce outside of the ElasticSearch source. Merging the entire config array into the url is dangerous and shouldn't be done.

Member

markstory replied Jan 19, 2013

I'll see if I can reproduce outside of the ElasticSearch source. Merging the entire config array into the url is dangerous and shouldn't be done.

This comment has been minimized.

Show comment Hide comment
@markstory

markstory Jan 20, 2013

Member

Running the following from a shell method doesn't trigger any errors or warnings:

$http = new HttpSocket();
$res = $http->get('https://github.com/markstory');
debug($res->isOk());

$res2 = $http->get('https://github.com/lorenzo');
debug($res2->isOk());

Additionally $http->request does not contain the context options. I'm inclined to believe that the ElasticSearch datasource is doing something silly here.

Member

markstory replied Jan 20, 2013

Running the following from a shell method doesn't trigger any errors or warnings:

$http = new HttpSocket();
$res = $http->get('https://github.com/markstory');
debug($res->isOk());

$res2 = $http->get('https://github.com/lorenzo');
debug($res2->isOk());

Additionally $http->request does not contain the context options. I'm inclined to believe that the ElasticSearch datasource is doing something silly here.

This comment has been minimized.

Show comment Hide comment
@ryantology

ryantology Jan 21, 2013

Sorry, it has only to do with the http->settings being re-used for the computation of the URI. It's not really a bug in Httpsocket as much as an unexpected change. This was the fix that I pushed to ElasticSource IdeaCouture/CakePHP-Elastic-Search-DataSource@4c7e669 As you can see there is already a edge case for the 'request' parameter being an array. Sorry for the witch hunt.

I do think it is a bit strange to change the settings array after the construct is called but it's not 'wrong'. It just makes it strange that the settings for one call are different if the same call is made twice.

Sorry, it has only to do with the http->settings being re-used for the computation of the URI. It's not really a bug in Httpsocket as much as an unexpected change. This was the fix that I pushed to ElasticSource IdeaCouture/CakePHP-Elastic-Search-DataSource@4c7e669 As you can see there is already a edge case for the 'request' parameter being an array. Sorry for the witch hunt.

I do think it is a bit strange to change the settings array after the construct is called but it's not 'wrong'. It just makes it strange that the settings for one call are different if the same call is made twice.

@lemon-lime

This comment has been minimized.

Show comment Hide comment
@lemon-lime

lemon-lime Mar 19, 2014

This modification breaks routes that include a urlencoded '/' in a parameter.

This modification breaks routes that include a urlencoded '/' in a parameter.

This comment has been minimized.

Show comment Hide comment
@markstory

markstory Mar 20, 2014

Member

That is unfortunate, I would think utf8 characters are more likely to be found in path segments over encoded / 's.

Member

markstory replied Mar 20, 2014

That is unfortunate, I would think utf8 characters are more likely to be found in path segments over encoded / 's.

This comment has been minimized.

Show comment Hide comment
@zoghal

zoghal Mar 20, 2014

Contributor

@lemon-lime For more reviews, please tell me an example of a url that failed

Contributor

zoghal replied Mar 20, 2014

@lemon-lime For more reviews, please tell me an example of a url that failed

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