Call load_callback for every fields value in ModulePersonalData. #4018

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
4 participants
@tristanlins
Contributor

tristanlins commented Feb 28, 2012

The ModulePersonalData calls the save_callback, but never the load_callback!

@tristanlins

This comment has been minimized.

Show comment Hide comment
@tristanlins

tristanlins Feb 28, 2012

Contributor

I added the onsubmit_callback and passes $this->User and $this as @Toflar sugested.

Contributor

tristanlins commented Feb 28, 2012

I added the onsubmit_callback and passes $this->User and $this as @Toflar sugested.

@Toflar

This comment has been minimized.

Show comment Hide comment
@Toflar

Toflar Apr 24, 2012

Member

Probably it was not really a good idea to pass $this->User. It's available anyway and the callbacks aren't consinstent in the FE and BE then.
@leofeyer thoughts? :-)

Member

Toflar commented Apr 24, 2012

Probably it was not really a good idea to pass $this->User. It's available anyway and the callbacks aren't consinstent in the FE and BE then.
@leofeyer thoughts? :-)

@leofeyer

This comment has been minimized.

Show comment Hide comment
@leofeyer

leofeyer Jun 22, 2012

Member

I agree that the calls should be the same in FE and BE. Can you please check 06ac7f3 and 7616ecf (branch "personal-data-callbacks") and get back to me whether it works?

Member

leofeyer commented Jun 22, 2012

I agree that the calls should be the same in FE and BE. Can you please check 06ac7f3 and 7616ecf (branch "personal-data-callbacks") and get back to me whether it works?

@leofeyer

This comment has been minimized.

Show comment Hide comment
@leofeyer

leofeyer Jul 23, 2012

Member

@contao/workgroup-core Please check the changes so I can merge the feature branch.

Member

leofeyer commented Jul 23, 2012

@contao/workgroup-core Please check the changes so I can merge the feature branch.

@aschempp

This comment has been minimized.

Show comment Hide comment
@aschempp

aschempp Jul 23, 2012

Member

I'm not sure why you're changing current behaviour? The save_callback is also passing the user. It should be very easy to distinguish TL_MOD in the callback, and $param->id will also be the same. We've also added $this at some point to have module information available.

One more thing I noticed, the variable should be called $varValue to follow Contao conventions.

Member

aschempp commented Jul 23, 2012

I'm not sure why you're changing current behaviour? The save_callback is also passing the user. It should be very easy to distinguish TL_MOD in the callback, and $param->id will also be the same. We've also added $this at some point to have module information available.

One more thing I noticed, the variable should be called $varValue to follow Contao conventions.

@tristanlins

This comment has been minimized.

Show comment Hide comment
@tristanlins

tristanlins Jul 24, 2012

Contributor

@aschempp But what about fields, that you fill with the load_callback? Thats how I find this problem, I tried to load a field value via the load_callback. Normally for these situations you need a save AND load callback, but only the save callback is triggered.
Thats just not the correct behaviour. Working with DCA records in FE editing should everytime call ALL callbacks, because you never can say which is needed in which situation.

Contributor

tristanlins commented Jul 24, 2012

@aschempp But what about fields, that you fill with the load_callback? Thats how I find this problem, I tried to load a field value via the load_callback. Normally for these situations you need a save AND load callback, but only the save callback is triggered.
Thats just not the correct behaviour. Working with DCA records in FE editing should everytime call ALL callbacks, because you never can say which is needed in which situation.

@aschempp

This comment has been minimized.

Show comment Hide comment
@aschempp

aschempp Jul 24, 2012

Member

I did not mean I don't want the callback. I want it like you implemented it, including the second (and third) parameters. Make the load_callback the same as the save_callback always was.

Member

aschempp commented Jul 24, 2012

I did not mean I don't want the callback. I want it like you implemented it, including the second (and third) parameters. Make the load_callback the same as the save_callback always was.

@tristanlins

This comment has been minimized.

Show comment Hide comment
@tristanlins

tristanlins Jul 24, 2012

Contributor

@aschempp sry I missunderstand you -.-
I think @leofeyer allready do the right changes in the 7616ecf commit?!

Contributor

tristanlins commented Jul 24, 2012

@aschempp sry I missunderstand you -.-
I think @leofeyer allready do the right changes in the 7616ecf commit?!

@aschempp

This comment has been minimized.

Show comment Hide comment
@aschempp

aschempp Jul 24, 2012

Member

He removed them! Thats the opposite of what I wanted. Please check the discussion from Yanick.

Member

aschempp commented Jul 24, 2012

He removed them! Thats the opposite of what I wanted. Please check the discussion from Yanick.

@leofeyer

This comment has been minimized.

Show comment Hide comment
@leofeyer

leofeyer Jul 30, 2012

Member

If this feature is supposed to make it into Contao 3.0, I need a very soon agreement and a code check by the @contao/workgroup-core. Otherwise I'll have to reassign the ticket to Contao 3.1.

Member

leofeyer commented Jul 30, 2012

If this feature is supposed to make it into Contao 3.0, I need a very soon agreement and a code check by the @contao/workgroup-core. Otherwise I'll have to reassign the ticket to Contao 3.1.

@aschempp

This comment has been minimized.

Show comment Hide comment
@aschempp

aschempp Aug 3, 2012

Member

I've submitted my recommended changes in #4585

  1. Follow Contao variable name conventions ($varValue not $value)
  2. Include second and third parameter on callback. This is useful because in the callback, $dc->id will still work and return the user ID. It also follows the existing save_callback, and you doubt you want to change (and break) that.

Apart from these things, I'm very happy with the changes. I've also checked the core, there are no existing callbacks that would be broken.

Member

aschempp commented Aug 3, 2012

I've submitted my recommended changes in #4585

  1. Follow Contao variable name conventions ($varValue not $value)
  2. Include second and third parameter on callback. This is useful because in the callback, $dc->id will still work and return the user ID. It also follows the existing save_callback, and you doubt you want to change (and break) that.

Apart from these things, I'm very happy with the changes. I've also checked the core, there are no existing callbacks that would be broken.

@leofeyer

This comment has been minimized.

Show comment Hide comment
@leofeyer

leofeyer Aug 6, 2012

Member

However, now the callbacks are inconsistent, because in the back end, they will pass $varValue and $dc and in the front end, they will pass $varValue, $this->User and $dc. This is not acceptable, so I will revert those changes.

Member

leofeyer commented Aug 6, 2012

However, now the callbacks are inconsistent, because in the back end, they will pass $varValue and $dc and in the front end, they will pass $varValue, $this->User and $dc. This is not acceptable, so I will revert those changes.

@aschempp

This comment has been minimized.

Show comment Hide comment
@aschempp

aschempp Aug 6, 2012

Member

That is not really true. First of all, the existing callbacks work the same, so it does make sense to keep it. Secondly, the idea was that $dc->id works the same in frontend and backend. It does now because $dc->id should return the user ID. The third parameter from the frontend is not a $dc, it's the module object. It allows to define custom settings into the module definition, and handle them inside the callback.

Member

aschempp commented Aug 6, 2012

That is not really true. First of all, the existing callbacks work the same, so it does make sense to keep it. Secondly, the idea was that $dc->id works the same in frontend and backend. It does now because $dc->id should return the user ID. The third parameter from the frontend is not a $dc, it's the module object. It allows to define custom settings into the module definition, and handle them inside the callback.

@leofeyer

This comment has been minimized.

Show comment Hide comment
@leofeyer

leofeyer Aug 6, 2012

Member

Ok, granted you have a point. But my callback looks like this:

<?php

function callback($varValue, \DataContainer $dc)
{
}

This does not work with your callback, right?

Member

leofeyer commented Aug 6, 2012

Ok, granted you have a point. But my callback looks like this:

<?php

function callback($varValue, \DataContainer $dc)
{
}

This does not work with your callback, right?

@leofeyer

This comment has been minimized.

Show comment Hide comment
@leofeyer

leofeyer Aug 6, 2012

Member

I have reverted my changes until we talked.

Member

leofeyer commented Aug 6, 2012

I have reverted my changes until we talked.

@leofeyer

This comment has been minimized.

Show comment Hide comment
@leofeyer

leofeyer Aug 6, 2012

Member

Implemented in ce4faf2.

Member

leofeyer commented Aug 6, 2012

Implemented in ce4faf2.

@leofeyer leofeyer closed this Aug 6, 2012

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