This is side issue split off from

#2542376: Update flag links for 'Make CSRF links cacheable'

Something in the back of my head makes me think I may be missing a more elegant solution, so I am just posting early and in public to see what gives.

So this change is very much up for discussion ... the final patch must change both FlagConformBaseFrom, FlaggingConfirmForm

In the interest of full disclosure I copied the solution from the devel module

\Drupal\devel\Form\ConfigEditor:buildCancelLinkUrl()

  /**
   * Builds the cancel link url for the form.
   *
   * @return Url
   *   Cancel url
   */
  private function buildCancelLinkUrl() {
    $query = $this->getRequest()->query;

    if ($query->has('destination')) {
      $options = UrlHelper::parse($query->get('destination'));
      $url = Url::fromUri('internal:/' . $options['path'], $options);
    }
    else {
      $url = Url::fromRoute('devel.configs_list');
    }

    return $url;
  }
CommentFileSizeAuthor
#14 toUrl-2542860-14.patch751 bytesmartin107
#13 toUrl-2542860-13.patch567 bytesmartin107
#8 simplify-2542860-8.patch4.18 KBmartin107
#5 createFromPathRemoved_2542860.5.patch2.05 KBsocketwench
createFromPath-0.patch961 bytesmartin107

Comments

socketwench’s picture

socketwench commented
Status: Needs review » Needs work

Ugh. Doesn't look like there is a more elegant solution. 

+++ b/src/Form/FlagConfirmBaseForm.php
@@ -48,9 +49,10 @@ abstract class FlagConfirmBaseForm extends ConfirmFormBase {
+      $options = UrlHelper::parse($query->get('destination'));

Are we parsing this to prevent XSS attacks? If so, we should add a comment since it looks a bit superfluous.

martin107’s picture

martin107 commented
Title: Replace calls to Url::createFromPath() » Url::createFromPath() has been removed.

Ugh. Doesn't look like there is a more elegant solution.

I did not express myself correctly ... at the very least I buried the lead....

The main thrust of the issue should be...

Url::createFromPath() has been removed - we need a solution.

@socketwench - I do accept your point ..... I am still looking for that elegant way forward :)

What is the etiquette here .. are calls to undefined functions critical or major?

joachim’s picture

joachim commented 
+++ b/src/Form/FlagConfirmBaseForm.php
@@ -48,9 +49,10 @@ abstract class FlagConfirmBaseForm extends ConfirmFormBase {
+    if ($query->has('destination')) {
+      $options = UrlHelper::parse($query->get('destination'));
+      return Url::fromUri('internal:/' . $options['path'], $options);

That seems horribly wordy!

Core is giving us the destination, and we're merely feeding it back to core. Why do we have to do all this intermediary faffing with it?

joachim’s picture

joachim commented

A quick search in core suggests need the not-very-well documented https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Routing!RedirectD...

socketwench’s picture

socketwench commented
Status: Needs work » Needs review
StatusFileSize
new createFromPathRemoved_2542860.5.patch2.05 KB

urlInfo() was also deprecated, so let's fix that too.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

You can actually delete all this. Just return the default cancel link.

See \Drupal\Core\Form\ConfirmFormHelper::buildCancelLink(), confirm forms use that to automatically handle the destination query argument. See \Drupal\Core\Form\ConfirmFormBase::buildForm().

martin107’s picture

martin107 commented
Assigned: Unassigned » martin107

@berdir thanks for the answer.

That way forward sorts out the cache contexts as well.

I will sort this out later today ...and patch as necessary.

martin107’s picture

martin107 commented
Assigned: martin107 » Unassigned
Status: Needs work » Needs review
StatusFileSize
new simplify-2542860-8.patch4.18 KB

1) FlagConfirmBaseForm has a name which is a little non-standard so I am changing it to fit in with the D8 convention

see FilerFormBase, LanguageFormBase, SearchPageFormBase.

2) The fact that we implement getCancelUrl() twice made me suspicious,

All the logic in FlagConfirmFormBase was being reimplemented in FlaggingConfirmBase
So I have simplified things by extending FlaggingConfirmBase from FlagConfirmBase so lots of code can be deleted.

If a reviewer has a problem with that ... I understand .. Is the answer to come up with a better base class name from which all flagForms and all flaggingForms can naturally extended?

3) $this->entity->urlInfo() is deprecated. $this->entity->toUrl() is the new thing in town.

There is no interdiff ... as I started from top of the tree.

As for manual testing I can assert both flag and unflag actions can be canceled and the user is directed back to the correct page.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Seems to me that FlaggingConfirmForm is trying to do the same as FlagConfirmForm/UnflagConfirmForm in a more generic way.

And it also looks like FlaggingConfirmForm is actually unused at the moment? So maybe we should just drop that?

martin107’s picture

martin107 commented
Assigned: Unassigned » martin107
Status: Needs review » Postponed

To criticise my own work - this is now 3 separate issues

1) FlaggingConfirmForm is unused and can be deleted.
2) FlagConfirmBaseForm - swap Base with Form
3) Url::createFromPath() is deprecated.

It is a pain to review ... so I am splitting into 3 issues. I will provide links soon.

This issue was originally (3) so I am postponing here until (1) and (2) are fixed.

martin107’s picture

martin107 commented
Related issues: +#2699633: FlagConfirmBaseForm - name change swap Base with Form, +#2699629: FlaggingConfirmForm is unused and can be deleted.
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Priority: Normal » Major

Splitting up makes sense.

But I'd recommend to do the actual bug fix (3) first. 1) is just dead code removal, 2) is refactoring, this fixes a fatal error.

Just only fix it for the form that we want to keep, ignore the one that we'll remove.

martin107’s picture

martin107 commented
Assigned: martin107 » Unassigned
Status: Postponed » Needs review
StatusFileSize
new toUrl-2542860-13.patch567 bytes

Ah I WAS being cautious -

1) I was trying to account for that fact that in [##2699629] a reviewer might make the case that FlaggingConformForm is the form that should be kept.
2) This patch will also clash with #2699633: FlagConfirmBaseForm - name change swap Base with Form

But you are correct let prioritize and get this in first.

martin107’s picture

martin107 commented
StatusFileSize
new toUrl-2542860-14.patch751 bytes

Missed a little thing.

socketwench’s picture

socketwench commented
Status: Needs review » Fixed

Thanks everyone!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.