Skip to content
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

#5328 adds phalcon version 3.4.x module #5329

Closed
wants to merge 7 commits into from

Conversation

chilimatic
Copy link

@chilimatic chilimatic commented Jan 8, 2019

#5328

I did only fix the interfaces and left the other code, besides the module name intact.

I don't mind better solutions but since phalcon is working on 4.x as their current release target and I did not want to impose any structure.

This is a 5 minutes patch, so if there is a method like an internal switch for different versions based on a parameter I am more than willing to adapt this PR.

src/Codeception/Module/Phalcon34x.php Outdated Show resolved Hide resolved
… adapter based on the internal phalcon version

 - changes private accessors for memory session
 - removes all fluff for memory-session override
/**
* @return string
*/
private function getMemorySessionForVersion() {

Choose a reason for hiding this comment

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

Opening brace should be on a new line

*/
private function getMemorySessionForVersion() {
if (strpos(\Phalcon\Version::get(), '3.4') !== false) {
return PhalconConnector\MemorySession34x::class;
Copy link
Member

Choose a reason for hiding this comment

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

This class will work with Phalcon 4.x too, right?
It would be better to use version_compare
version_compare(\Phalcon\Version::get(), '3.4.0') >= 0)

Copy link
Author

Choose a reason for hiding this comment

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

It's unclear if it does work with 4.x I'll compile and test it to check. Good point though.

Copy link
Member

Choose a reason for hiding this comment

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

It is more likely to work than the old class.

Copy link
Author

@chilimatic chilimatic Jan 8, 2019

Choose a reason for hiding this comment

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

yes :D probability says it's likely. still if I don't need heuristics I won't use them :) 10 mins :) and we know it for sure I already changed it. I just verify it

Copy link
Author

Choose a reason for hiding this comment

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

@Naktibalda currently there is not even an Session adapter interface in branch 4.0.x so let @sergeyklay decide how to proceed :). Maybe it's not consolidated atm.

Copy link
Member

Choose a reason for hiding this comment

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

Since v4.0.0 session adapters must implement the SessionHandlerInterface interface. There is at least one fundamental difference:

// Phalcon 3.x
public function destroy($removeData = false);

// Phalcon 4.x
public function destroy(string $session_id): bool;

Most likely MemorySession34x will not work with Phalcon 4.x.

…eaks (could work trait/composition based though)
…itialization process has not been triggered

 - adds extra check if the client does actually exist (context free testing | helper | utilities)
*/
private function getMemorySessionForVersion() {
if (strpos(\Phalcon\Version::get(), '3.4') !== false) {
return PhalconConnector\MemorySession34x::class;
Copy link
Member

Choose a reason for hiding this comment

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

Since v4.0.0 session adapters must implement the SessionHandlerInterface interface. There is at least one fundamental difference:

// Phalcon 3.x
public function destroy($removeData = false);

// Phalcon 4.x
public function destroy(string $session_id): bool;

Most likely MemorySession34x will not work with Phalcon 4.x.

@chilimatic
Copy link
Author

invalid due to zephir compiler to phalcon target version.
using zephir 10.x + phalcon 3.4.x resolves the problem

@chilimatic chilimatic closed this Jan 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants