Replaced $this->client with $this->getClient().

[stereomon]

Using the client property directly blocks us from a proper arrangement of test cases in conjunction with Symfony's HttpKernel.

Usually, the client property is set very early in the testing process e.g. in _before. In the case of HttpKernel usage, this will also trigger the load of e.g. the EventDispatcher which blocks us from manipulating the EventDispatcher inside of test methods.

This change should not affect the usage of the client property as before but gives us more flexibility in our test setup.

[DavertMik]

even the code is fine I didn't notice a difference from the previous behavior you are using getter instead of a property, but I didn't see how the logic has changed in it and how is it related to event dispatcher?

[stereomon]

@DavertMik The difference is that with this change you have the ability to prepare your test case before the client is initialized. As explained, usually the client property is set in _before, _beforeSuite, or in the initialize method of a module. This is happening before the test case is executed. We need the ability to initialize the client later and only when we use it e.g. with $i->amOnPage() to be able to arrange the test case before the client is used in our test case.

I encountered this problem as we wanted to add listeners to the EventDispatcher inside of the test case but it was not possible as the client which uses the Symfony's Kernel which uses the EventDispatcher was initialized too early. We are using Pimple 3, and with Pimple 3 you will receive a service frozen exception when you want to change a service that was already retrieved from the container. That is the case when you get the Kernel (while setting the client property) in e.g. _before thus we aren't able to change the EventDispatcher in our test case.

Current

Before the test case:

• execute modules in _before, _beforeSuite etc.
  • One module initializes the client with Kernel before the test case (EventDispatcher is now frozen because it was retrieved from the container)

In the test case: (public function test*)

• Arrange: change the EventDispatcher and add it to the container -> frozen exception

After change

Before the test case:

• execute modules in _before, _beforeSuite etc.

In the test case: (public function test*)

• Arrange: change the EventDispatcher and add it to the container
• Act: e.g. $i->amOnPage()
  • now the client will be initialized here as we use the getter
• Assert

[SamMousa]

Can't you use the lifecycle methods to just create a new client? This feels like a hack to me. A getter for a public variable that is not lazily created doesn't add anything. I suspect you want to override the getter in a client class, but because the client is public this would cause unexpected behavior for people expecting to be able to write the public $client property.

In my opinion changes like this should introduce a setter as well, if that makes sense for the API, and more importantly should make the property private.

Note that this is a break in BC.

[stereomon]

@SamMousa The normal lifecycle can't be used as described above. You're right, I need to override the getter in my own client class to achieve the expected behavior.

Currently, you can use the property as before. Changing the property's visibility would be a BC break. IMO the whole class needs a major refactoring but this is out of scope.

[SamMousa]

You're right, I need to override the getter in my own client class to achieve the expected behavior.

Couldn't you implement this via a proxy pattern? Ie use a client that is a proxy to the actual client and allows you to achieve the desired behavior?

IMO the whole class needs a major refactoring but this is out of scope.

Agree with both these points ;-)

[stereomon]

Probably, a Proxy could be implemented but IMO a slight refactoring as done in this PR would be the better approach as others could also benefit. Having a Proxy on our side puts maintenance effort to us which should not be a solution here.

We could also deprecate the $client property and add a setter for it as you said.

[SamMousa]

Probably, a Proxy could be implemented but IMO a slight refactoring as done in this PR would be the better approach as others could also benefit.

This is true in theory, but the way it's implemented means it will also cause unexpected behavior of which the maintenance effort would be on us.

If a refactoring improves the code I'm all for it, but if it doesn't then I prefer to have the fix (and effort of its maintenance) on your side. That said I'm definitely in favor of deprecating the $client but that is a bigger thing that will require a new version to be released as well as changes for lots of modules.

[stereomon]

it will also cause unexpected behavior

Can you explain that to me, TBH I don't see what you mean. The class behaves as before + covers my and probably the case of others. The proxy seems not to be a good idea, especially with the private method.

How about suggesting changes to this PR and we change it in an acceptable way?

@DavertMik what's your opinion?

[SamMousa]

Unexpected behavior happens if someone using the child class writes to the public variable and the getter interacts in a different way than the current implementation.

Can you show the child implementation you're considering?

[stereomon]

The getClient() method returns you the value of the $client property and this is the same as before, there has nothing changed.

[stereomon]

@DavertMik Any news about this topic?

[dereuromark]

ping @DavertMik we need to fork and maintain our own repo here otherwise, and would really prefer not doing that if possible.

It is true that this is BC if the wrapper method only returns the object as non nullable value. if it is about the naming, one could also use just client() or alike. The result is the same.

[SamMousa]

We should really NOT merge this. As I have outlined above, it creates a brittle API that will come back to bite us.

Looking into this some more, I think you should use the reconfiguration flow that uses onReconfigure https://github.com/Codeception/module-symfony/blob/master/src/Codeception/Module/Symfony.php#L235 internally to recreate the kernel. FYI: I don't use symfony, but am familiar with the Yii module

https://codeception.com/docs/06-ModulesAndHelpers#runtime-configuration

Specifically the Symfony module offers rebootClientKernel to reboot the kernel after reconfiguration.

[DavertMik]

@dereuromark the question is not in name but in the signature. changing a property call to a method call will affect all other modules relying on this method. And this will be modules of all frameworks, REST, PhpBrowser.

The only solution would be to implement __get() method that will magically expose client property.

If this is an option I'm ok with this solution.

P.S. Nowadays I would prefer to use getClient as well but because of BC we can't do this

[SamMousa]

The only solution would be to implement __get() method that will magically expose client property.

That's not a solution; it might do the same in some cases but it is definitely not backwards compatible. It also changes generic class behavior and requires a whole load of workarounds.

I think this can be solved with the reconfiguration we already support, in which case this PR is not needed at all. I propose @dereuromark first tries to solve his problem that way.

[DavertMik]

Ok, I will agree with @SamMousa because maintaining this set of connecting modules and changing the public API on the fly is bad decision.