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

Implement getInternalsDomains methods for all frameworks #2396

Closed
Naktibalda opened this Issue Sep 22, 2015 · 24 comments

Comments

Projects
None yet
6 participants
@Naktibalda
Member

Naktibalda commented Sep 22, 2015

This is a follow up to #2371

I created a new branch: https://github.com/Codeception/Codeception/tree/feature-get-internal-domains
All frameworks that support multiple domain names in tests must implement getInternalDomain method.

ToDo

  • Laravel4
  • Laravel5
  • Lumen
  • Silex
  • Symfony2
  • Phalcon1
  • Phalcon2
  • Yii1
  • Yii2
@janhenkgerritsen

This comment has been minimized.

Show comment
Hide comment
@janhenkgerritsen

janhenkgerritsen Sep 23, 2015

Contributor

Thanks for setting up the branch, I will try to implement this as soon as possible.

Contributor

janhenkgerritsen commented Sep 23, 2015

Thanks for setting up the branch, I will try to implement this as soon as possible.

@Naktibalda

This comment has been minimized.

Show comment
Hide comment
@Naktibalda

Naktibalda Sep 23, 2015

Member

A better place for default implementation of getInternalDomain method would be a Codeception\Lib\Framework class or a new common parent of all Connector classes.

And we could get rid of interface.

Member

Naktibalda commented Sep 23, 2015

A better place for default implementation of getInternalDomain method would be a Codeception\Lib\Framework class or a new common parent of all Connector classes.

And we could get rid of interface.

@Naktibalda

This comment has been minimized.

Show comment
Hide comment
@Naktibalda

Naktibalda Sep 26, 2015

Member

I implemented getInternalDomains for ZF2,
code: #2405
tests: Naktibalda/codeception-zf2-tests#2

An important observation is that domains are not limited to literal values, so in_array is not enough.
ZF2 supports placeholders in domain route and uses regex for matching.

My implementation of getInternalDomains returns array of patterns.
Can you do something similar for Laravel?

Member

Naktibalda commented Sep 26, 2015

I implemented getInternalDomains for ZF2,
code: #2405
tests: Naktibalda/codeception-zf2-tests#2

An important observation is that domains are not limited to literal values, so in_array is not enough.
ZF2 supports placeholders in domain route and uses regex for matching.

My implementation of getInternalDomains returns array of patterns.
Can you do something similar for Laravel?

@DavertMik DavertMik added the Discuss label Oct 1, 2015

@janhenkgerritsen

This comment has been minimized.

Show comment
Hide comment
@janhenkgerritsen

janhenkgerritsen Oct 1, 2015

Contributor

I implemented the getInternalDomains() method for the Laravel 5 module, and I also added tests for the functionality to the sample application: janhenkgerritsen/codeception-laravel5-sample@0687148.

To get this to work I had to use reflection. So I introduced a Codeception\Util\ReflectionHelper class. This class has two static methods. The first method is the readPrivateProperty method of the PropertyAccess class. The second method is a new method called invokePrivateMethod. I deprecated the PropertyAccess::readPrivateProperty method for now, and replaced the implementation, it now delegates to the method on the ReflectionHelper class.

If this feature branch is merged into the 2.1 branch we should replace all references to PropertyAccess::readPrivateProperty with ReflectionHelper::readPrivateProperty. But should we also remove the PropertyAccess class, or do we keep it for backwards compatability? I think we can remove it, because it hasn't been in the code very long and with normal Codeception usage you would not use this class. So mentioning this in the changelog should be enough, and the fix is very easy for people that do run into problems. What do you guys think?

Contributor

janhenkgerritsen commented Oct 1, 2015

I implemented the getInternalDomains() method for the Laravel 5 module, and I also added tests for the functionality to the sample application: janhenkgerritsen/codeception-laravel5-sample@0687148.

To get this to work I had to use reflection. So I introduced a Codeception\Util\ReflectionHelper class. This class has two static methods. The first method is the readPrivateProperty method of the PropertyAccess class. The second method is a new method called invokePrivateMethod. I deprecated the PropertyAccess::readPrivateProperty method for now, and replaced the implementation, it now delegates to the method on the ReflectionHelper class.

If this feature branch is merged into the 2.1 branch we should replace all references to PropertyAccess::readPrivateProperty with ReflectionHelper::readPrivateProperty. But should we also remove the PropertyAccess class, or do we keep it for backwards compatability? I think we can remove it, because it hasn't been in the code very long and with normal Codeception usage you would not use this class. So mentioning this in the changelog should be enough, and the fix is very easy for people that do run into problems. What do you guys think?

@janhenkgerritsen

This comment has been minimized.

Show comment
Hide comment
@janhenkgerritsen

janhenkgerritsen Oct 1, 2015

Contributor

Just implemented the method for the Laravel 4 module. I don't have the time to implement the method for the Lumen module today. Will do that asap.

Contributor

janhenkgerritsen commented Oct 1, 2015

Just implemented the method for the Laravel 4 module. I don't have the time to implement the method for the Lumen module today. Will do that asap.

@janhenkgerritsen

This comment has been minimized.

Show comment
Hide comment
@janhenkgerritsen

janhenkgerritsen Oct 2, 2015

Contributor

Lumen does not support domain routing, so I deleted the getInternalDomains() method from the Lumen module.

Contributor

janhenkgerritsen commented Oct 2, 2015

Lumen does not support domain routing, so I deleted the getInternalDomains() method from the Lumen module.

@raistlin

This comment has been minimized.

Show comment
Hide comment
@raistlin

raistlin Oct 3, 2015

Member

Implementation for Symfony2 #2420

Member

raistlin commented Oct 3, 2015

Implementation for Symfony2 #2420

@janhenkgerritsen

This comment has been minimized.

Show comment
Hide comment
@janhenkgerritsen

janhenkgerritsen Oct 4, 2015

Contributor

Is someone familiar with Silex? I am not, but if nobody else is I can look into the implementation for the Silex module. Should not be too difficult I think.

Contributor

janhenkgerritsen commented Oct 4, 2015

Is someone familiar with Silex? I am not, but if nobody else is I can look into the implementation for the Silex module. Should not be too difficult I think.

@janhenkgerritsen

This comment has been minimized.

Show comment
Hide comment
@janhenkgerritsen

janhenkgerritsen Oct 12, 2015

Contributor

@samdark Does Yii support domain routing? And if so, can you implement the functionality for this feature for the Yii modules?

I will look into the implementation for the Silex module.

Contributor

janhenkgerritsen commented Oct 12, 2015

@samdark Does Yii support domain routing? And if so, can you implement the functionality for this feature for the Yii modules?

I will look into the implementation for the Silex module.

@janhenkgerritsen

This comment has been minimized.

Show comment
Hide comment
@janhenkgerritsen

janhenkgerritsen Oct 12, 2015

Contributor

I just implemented the getInternalDomains method for the Silex module.

Contributor

janhenkgerritsen commented Oct 12, 2015

I just implemented the getInternalDomains method for the Silex module.

@sergeyklay

This comment has been minimized.

Show comment
Hide comment
@sergeyklay

sergeyklay Oct 18, 2015

Member

Implementation for Phalcon1 #2457 (Phalcon2 extends Phalcon1)

Member

sergeyklay commented Oct 18, 2015

Implementation for Phalcon1 #2457 (Phalcon2 extends Phalcon1)

@Naktibalda

This comment has been minimized.

Show comment
Hide comment
@Naktibalda

Naktibalda Nov 28, 2015

Member

Are we waiting for anything?

Member

Naktibalda commented Nov 28, 2015

Are we waiting for anything?

@janhenkgerritsen

This comment has been minimized.

Show comment
Hide comment
@janhenkgerritsen

janhenkgerritsen Nov 28, 2015

Contributor

We are waiting on the implementation for Yii.

Contributor

janhenkgerritsen commented Nov 28, 2015

We are waiting on the implementation for Yii.

@Naktibalda

This comment has been minimized.

Show comment
Hide comment
@Naktibalda

Naktibalda Dec 1, 2015

Member

@samdark are you aware that you are blocking this process?

Member

Naktibalda commented Dec 1, 2015

@samdark are you aware that you are blocking this process?

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Dec 1, 2015

Collaborator

Yes. Can't do anything about it atm. Busy with Yii 2.0.7 release.

Collaborator

samdark commented Dec 1, 2015

Yes. Can't do anything about it atm. Busy with Yii 2.0.7 release.

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Dec 2, 2015

Collaborator

Found some time. I have a problem implementing it. Both Yii 1.1 and Yii 2.0 support wildcards for domain names such as:

http://<name>.example.com/test
http://example.<name>.com/test

What should be returned in this case?

Collaborator

samdark commented Dec 2, 2015

Found some time. I have a problem implementing it. Both Yii 1.1 and Yii 2.0 support wildcards for domain names such as:

http://<name>.example.com/test
http://example.<name>.com/test

What should be returned in this case?

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Dec 2, 2015

Collaborator

Code for Yii 1.1 and 2.0 is in the branch but parametrized routes are now simply ignored :(

Collaborator

samdark commented Dec 2, 2015

Code for Yii 1.1 and 2.0 is in the branch but parametrized routes are now simply ignored :(

@Naktibalda

This comment has been minimized.

Show comment
Hide comment
@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Dec 2, 2015

Collaborator

OK. Btw., does Silex module properly escape dots in regex generated?

Collaborator

samdark commented Dec 2, 2015

OK. Btw., does Silex module properly escape dots in regex generated?

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Dec 3, 2015

Collaborator

Finished Yii1 and Yii2 modules. getInternalDomains() of these returns ready to use properly escaped regexes such as:

/^http\://example\.com$/u
/^\w+\.example\.com$/u
/^[^\/]+\.example\.com$/u
Collaborator

samdark commented Dec 3, 2015

Finished Yii1 and Yii2 modules. getInternalDomains() of these returns ready to use properly escaped regexes such as:

/^http\://example\.com$/u
/^\w+\.example\.com$/u
/^[^\/]+\.example\.com$/u
@janhenkgerritsen

This comment has been minimized.

Show comment
Hide comment
@janhenkgerritsen

janhenkgerritsen Dec 3, 2015

Contributor

I updated the regex for the Silex module by adding a call to preg_quote, good catch.

Contributor

janhenkgerritsen commented Dec 3, 2015

I updated the regex for the Silex module by adding a call to preg_quote, good catch.

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Dec 3, 2015

Collaborator

Checked travis errors. Seems these are releated with what already fixed in master, not with new code.

Collaborator

samdark commented Dec 3, 2015

Checked travis errors. Seems these are releated with what already fixed in master, not with new code.

@Naktibalda

This comment has been minimized.

Show comment
Hide comment
@Naktibalda

Naktibalda Dec 3, 2015

Member

This branch can't be merged to 2.1 automatically. Could somebody rebase it?

Member

Naktibalda commented Dec 3, 2015

This branch can't be merged to 2.1 automatically. Could somebody rebase it?

@janhenkgerritsen

This comment has been minimized.

Show comment
Hide comment
@janhenkgerritsen

janhenkgerritsen Dec 7, 2015

Contributor

I merged this branch into 2.1 in #2611. Also deleted the feature branch.

Contributor

janhenkgerritsen commented Dec 7, 2015

I merged this branch into 2.1 in #2611. Also deleted the feature branch.

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