Skip to content

Update Connector to support _ in the broker uri - #66

Merged
jmglsn merged 3 commits into
stomp-php:masterfrom
campru:master
Dec 21, 2016
Merged

Update Connector to support _ in the broker uri#66
jmglsn merged 3 commits into
stomp-php:masterfrom
campru:master

Conversation

@campru

@campru campru commented Dec 19, 2016

Copy link
Copy Markdown
Contributor

Support underscore in the failover host name and parameters in the url. This issue provokes that if you are using docker and the systems are generated automatically it doesn't work because of the _

Support underscore in the failover host name and parameters in the url
@staabm

staabm commented Dec 19, 2016

Copy link
Copy Markdown
Member

could you add a unit test with a example of such a hostname to prevent regressions in the future?

@campru

campru commented Dec 19, 2016

Copy link
Copy Markdown
Contributor Author

Yes, no problem. I have already added a UT where the host contains underscore

Comment thread src/Stomp/Network/Connection.php Outdated
$this->connectTimeout = $connectionTimeout;
$this->context = $context;
$pattern = "|^(([a-zA-Z0-9]+)://)+\(*([a-zA-Z0-9\.:/i,-]+)\)*\??([a-zA-Z0-9=&]*)$|i";
$pattern = "|^(([a-zA-Z0-9]+)://)+\(*([a-zA-Z0-9\.:/i,-_]+)\)*\??([a-zA-Z0-9=&_]*)$|i";

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you also added the _ as allowed character within connection properties... was it intended?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in other words, your unit test should also pass when only adding the first underscore

@campru campru Dec 19, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I was not intented. I did another commit to revert this change. Now in the pull request the last _ shouldn't appear

Comment thread src/Stomp/Network/Connection.php Outdated
$this->connectTimeout = $connectionTimeout;
$this->context = $context;
$pattern = "|^(([a-zA-Z0-9]+)://)+\(*([a-zA-Z0-9\.:/i,-]+)\)*\??([a-zA-Z0-9=&]*)$|i";
$pattern = "|^(([a-zA-Z0-9]+)://)+\(*([a-zA-Z0-9\.:/i,-_]+)\)*\??([a-zA-Z0-9=&_]*)$|i";

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jmglsn why do we allow the protocol (([a-zA-Z0-9]+)://)+ once or more (trailling +)?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@staabm this is not wanted and can be dropped.

@staabm

staabm commented Dec 19, 2016

Copy link
Copy Markdown
Member

the change looks good to me, thx.

@campru

campru commented Dec 19, 2016

Copy link
Copy Markdown
Contributor Author

Thank you. I would like to know one this change is ready to be merged into master, how do you manage to generate a new tag with these changes? Thank you to take a look so fast :)

@staabm

staabm commented Dec 19, 2016

Copy link
Copy Markdown
Member

Either @jmglsn has a few mintues to give his opinion within the next few days or I will take care of it at the end of this week.

In the meantime you could use your own fork[1] in case you rely on this change in your project

[1] http://stackoverflow.com/questions/13498519/how-to-require-a-fork-with-composer

@campru

campru commented Dec 19, 2016

Copy link
Copy Markdown
Contributor Author

Great. Thank you for everything

@jmglsn

jmglsn commented Dec 20, 2016

Copy link
Copy Markdown
Member

@campru Thank you for this PR!
@staabm I'll back port this for the version 3.0 branch as well. The allowed + can be dropped, but this is ok for the moment.

@jmglsn
jmglsn merged commit 185d34f into stomp-php:master Dec 21, 2016
@jmglsn

jmglsn commented Dec 21, 2016

Copy link
Copy Markdown
Member

It's included in 4.2.1.

@campru

campru commented Dec 28, 2016

Copy link
Copy Markdown
Contributor Author

Thank you very much!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants