search

diem-project / diem

Diem CMF CMS for symfony 1.4
http://diem-project.org/
MIT License
184 stars 85 forks source link

Fix double widget action execution #420

Closed eXtreme closed 12 years ago

eXtreme commented 12 years ago

See: https://groups.google.com/d/topic/diem-users/TBO6imDfC4U/discussion

etherkye commented 12 years ago

I'm in that ;)

Thats not how to fix it. Also, you deleted recent changes made by steph to improve the efficiency of the code.

Your change will mean that actions are not called before the page starts to generate, which means if you have a flash ->setFlash() in an action, you can not check that before the widget linked to it is called. This is a problem.

eXtreme commented 12 years ago

@etherkye that was the decision in the thread ;)

but you are right, I've forgot about flashes.. So right know I only see a solution which has a register of executed widget actions to prevent double execution.

etherkye commented 12 years ago

You need to keep the bit you removed, and take it from the other area!

eXtreme commented 12 years ago

No, we can't. "the other area" makes execution of the action ALSO when:

  1. a widget is dropped onto zone in front editor
  2. a widget is previewed in front editor
  3. a widget is called programmatically by dm_get_widget() helper
etherkye commented 12 years ago
  1. Forms do not load when dropped, you have to refresh the page to see them. So this is not correct.
  2. I'm sure this can be fixed
  3. Then move it out of where it is and in to dm_get_widget()

Also, at the moment if you use dm_get_widget you'll notice that cache:static or cache:true on the widget that calls that, does NOT cache the JS or CSS called from that widget, so it only works once. So dm_get_widget() needs to be fixed anyway.

eXtreme commented 12 years ago

Ad 1. Erm.. I've just installed dmContactPlugin and dropped the widget and it is shown with the form. ;) And, well, forms are not the only thing you can do in widget actions.

Regarding the cache.. I don't see any solution here, we just should not cache a widget with dm_get_widget call.

But, well, yes I think I can revert the commiit and just make an action call directly in the helper (and so remove the call in dmFrontPageBaseHelper). We can see what side effects it's gonna cause ;)

etherkye commented 12 years ago

Thank you.

And this is Kye Etherton incase you hadn't guessed.

eXtreme commented 12 years ago

@etherkye Oh.. ;) I hadn't, sorry ;)

I will prepare a new commit later today.

eXtreme commented 12 years ago

And now? ;)

etherkye commented 12 years ago

That looks a lot lot better.

Please can you run the testing suite on that.

eXtreme commented 12 years ago

@etherkye well, I'm not sure my webserver is all fine on linux but I get this result of testing suite:

Failed 39/113 test scripts, 65.49% okay. 150/2276 subtests failed, 93.41% okay.

However it is exactly the same result I get after running the testing suite without this patch, so... I guess it does not break anything else ;)

etherkye commented 12 years ago

Meh, sounds about right.

Kye Etherton IT Manager Pimlico Plumbers T: 020 7928 8888 F: 020 7928 3333 http://www.Pimlicoplumbers.com

The contents of this e-mail (including any attachments) are confidential, for the recipient only and may be legally privileged. If you are not the intended recipient you should not print, copy or distribute this e-mail or use it or its contents or copy or publish its contents to any other person. If you have received this e-mail in error please notify the sender immediately and delete it from your system.

-----Original Message----- From: Jacek Jędrzejewski [mailto:reply@reply.github.com] Sent: 12 December 2011 15:45 To: Kye Etherton Subject: Re: [diem] Fix double widget action execution (#420)

@etherkye well, I'm not sure my webserver is all fine on linux but I get this result of testing suite:

Failed 39/113 test scripts, 65.49% okay. 150/2276 subtests failed, 93.41% okay.

However it is exactly the same result I get after running the testing suite without this patch, so... I guess it does not break anything else ;)

Reply to this email directly or view it on GitHub: https://github.com/diem-project/diem/pull/420#issuecomment-3107591

eXtreme commented 12 years ago

OK So I can do the merge, if you want

etherkye commented 12 years ago

Can you? Or do we need to wait for steph?

Kye Etherton IT Manager Pimlico Plumbers T: 020 7928 8888 F: 020 7928 3333 http://www.Pimlicoplumbers.com

The contents of this e-mail (including any attachments) are confidential, for the recipient only and may be legally privileged. If you are not the intended recipient you should not print, copy or distribute this e-mail or use it or its contents or copy or publish its contents to any other person. If you have received this e-mail in error please notify the sender immediately and delete it from your system.

-----Original Message----- From: Jacek Jędrzejewski [mailto:reply@reply.github.com] Sent: 13 December 2011 16:01 To: Kye Etherton Subject: Re: [diem] Fix double widget action execution (#420)

OK So I can do the merge, if you want

Reply to this email directly or view it on GitHub: https://github.com/diem-project/diem/pull/420#issuecomment-3124855

eXtreme commented 12 years ago

It's not a problem, I have write permissions to diem repo

etherkye commented 12 years ago

Go for it then

Kye Etherton IT Manager Pimlico Plumbers T: 020 7928 8888 F: 020 7928 3333 http://www.Pimlicoplumbers.com

The contents of this e-mail (including any attachments) are confidential, for the recipient only and may be legally privileged. If you are not the intended recipient you should not print, copy or distribute this e-mail or use it or its contents or copy or publish its contents to any other person. If you have received this e-mail in error please notify the sender immediately and delete it from your system.

-----Original Message----- From: Jacek Jędrzejewski [mailto:reply@reply.github.com] Sent: 13 December 2011 16:03 To: Kye Etherton Subject: Re: [diem] Fix double widget action execution (#420)

It's not a problem, I have write permissions to diem repo

Reply to this email directly or view it on GitHub: https://github.com/diem-project/diem/pull/420#issuecomment-3124885