Two of us have accidentally deleted html blocks within an hour of each other. It could definitely do with having the warning return! Thank you.

Has a confirm box now, as well as moved the delete button to the far right.

Although this issue may have been fixed at some point, it currently affects Moodle 2.3, as reported by Fernanda in http://moodle.org/mod/forum/discuss.php?d=206151

Issue size: M

Hi Andrian, all seems ok to me

Thanks David for having a look at this for me.
Submitting for peer review.

The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

TIA and ciao

Sorry Adrian,

I'm sending this back this week. You should not/can not use globals within a the block actions.
Have a look at what process_url_edit is doing which is also producing Output.
You need to produce a new moodle_page and use it to generate an output renderer that is related to that page.
The problem stems from that fact that you are within core_renderer::header when those block actions are being processed and are already deep within the output API and settings should have been finalised.

A couple of things to consider as well:

1. You should be using $block->get_title() rather than $block->title.
2. I think it would be worth testing this the multilang filter enabled and with an HTML block with a multilang title. That is the most complex situation a block title can get itself into.
3. Using confirm is not a great option. Block editing actions are designed to work on any page that has blocks. That page may have a param confirm already that would cause a conflict here. Best to stick with the bui_ prefix I think (that is there to help try keep it unique).
4. Don't forget to strip the confirm from the url when you are done with the action: $this->page->ensure_param_not_in_url

By the way and just out of curiosity did you find whether we originally had this functionality and if so when it was lost?

Cheers
Sam

Oh never mind about the last curiosity, I just looked at the age of this issue, I thought we already supported this in 1.9 but clearly I was wrong.

Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

I've made the changes that Sam has suggested.
I'm not exactly sure about my placement of
$this->page->ensure_param_not_in_url (delete_form.php line: 80) So if that could get some extra attention I would be very happy.

Thanks.

Hi Adrian,

About ensure_param_not_in_url(), as you said the call only seems to be necessary when the param exists, after the delete confirmation and the blocks_delete_instance() execution.

I'm not sure if it's necessary to create a form to display the confirm box because there are no form elements to display, maybe the OUTPUT->confirm() could be included in a renderer or in process_url_delete(), but following the process_url_edit() way to do it, to overwrite $PAGE and $OUTPUT as Sam proposes.

About the form, with this patch block plugins has the option to overwrite the default blocks/block_delete_form creating a new block_BLOCKNAME_delete_form; just asking, this kind of additions should be documented in MDocs? Adding to this, it could be useful to have more info in block_delete_form->__construct, maybe type hintings and/or method description

Thanks for the comments David.

• I've moved the ensure_param_not_in_url() function to its appropriate spot.
• I had a talk to Rajesh and he doesn't see any problem with using an mform to create a yes no confirmation page.
• I changed a couple of the parameters being passed to the delete_form so as to not pass unused information.

Submitting for integration review.

Hi Adrian,

I've just been looking at this and I've decided to send it back once more sorry.

There are a couple of things that I don't like about this solution when I sit down and look at it.

1. block_delete_form is inheriting from moodleform but is not acting like a form.
2. blocks don't need to be able to override the delete form like they do for the action form.

I think I get where you were heading with this but I think you've approached it incorrectly.
In concept the moodleform library is not supposed to have anything at all to do with page output. It should only be concerning itself with the form it is representing. That is how that library (quickforms+moodleform) is designed and is how it should be used.
The point of allowing a form to be overridden is so that its specification can alter the definition of the form, implement validation etc. Under your present use it is just a wrapper to output.
Its also worth noting that confirming the deletion of a block should never need to change, simply confirm yes or no. I can't think of a use case in which a blocks would need to override form and I really don't think it is worth doing.

What I think needs to happen here is pretty simple.

1. Copy the code from block_delete_form::definition process_url_delete and do away with delete_form.php.
2. Clean up that process_url_delete method within that !$confirmdelete condition.
3. Make sure after any $output->footer() call within that function that exit is called to make sure nothing more can occur.

Cheers
Sam

Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

Okay,
What I've done this time around.

1. Removed the form for deleting the block.
2. Taken the code from that page and included it into the function in the blocklib.
3. Tidied up the !$confirmdelete condition.
4. Added an exit after $OUTPUT->footer() call so that nothing else happens after the form is displayed.

Heading to peer review.

Hi Adrian,

All looks ok for me

Thanks David for your time and patience with going through this code and finding things that I missed.

Third time lucky for integration perhaps.

The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

TIA and ciao

The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.

This is a bulk-automated message, so if you want to blame some-body/thing/where, don't do it here (use git instead) :-D :-P

Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

TIA and ciao

Hi Adrian,

I am happy with the way this looks now thanks.
Could you please produce branches for the versions you want this to land on and then I'll integrate.

Many thanks
Sam

Hi Sam,

I tend to think that it's more of an improvement than a bug fix, so I'm inclined to just leave it in master, but I'm quite willing to create other branches for it if you think it should be back ported.

Integrated (23 & master), thanks!

Notes:

• The $PAGE = ... trick is really, really awful, but.
• Just guessing if, instead of defaulting to "admin" layout, we couldn't continue using the same, so blocks continue being shown.
• I've added one extra commit about $PAGE and $OUTPUT missing global declaration.
• Finally I've backported it to 2.3, because it seems to be a nice safety feature and we are still with .1). I was no intrepid enough to backport it to 22_STABLE.
• ui_change and docs_required labels added.

Ciao

Tested under 23 and master, with teacher and admin, both "yes" and "no". Everything seems to work ok. Passing.

Created MDL-35294 about to bring ajax facilities to the block buttons.

Many thanks for the hard work.

These changes have been spread upstream and are already available in the git and cvs repositories.

Ciao

Hurray!

I'm removing the docs_required label, even though I haven't actually written anything in the docs about this issue, since I don't think it needs documenting. If anyone thinks differently, and can suggest where in the docs some info should be added, please shout!

Perfect Helen,

I think it is better to add labels by excess than missing them, and then let the specialists to decide if it's worth the documentation.

In this case I was thinking that, perhaps, there is some place where the deletion of blocks is documented (with some warning about the deletion happening without confirmation or so) and it now has the new confirmation step. That's the reason I added the ui_change and also the docs_required labels.

In fact, I'm changing the title of this issue as far as it's global and not only html-block related.

Of course, if you think there is another better strategy to decide the labeling, let me know, 100%. Else, I'll continue current way.

Thanks and ciao

Just joining in here to say with regards to documentation I Very Much agree that "it's better to add labels by excess than missing them"

I agree too, that it's better to have ui_change and docs_required labels in too many places than not enough.

Eloy, thanks a lot for adding them everywhere. Please keep it up!