New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the return value to the Cancel option #20144

Merged
merged 3 commits into from May 12, 2018

Conversation

Projects
None yet
7 participants
@roland-d
Contributor

roland-d commented Apr 11, 2018

Summary of Changes

This change adds the behavior of being able to use a return URL on the Cancel button. This feature already exists on the Save & Close button and makes sense to me to add it to the other functions as well.

This is not something users will use manually but programmers can use it to make sure users go back to where they came from. It adds consistency to the workflow.

Testing Instructions

We are going to assume you reached the article via Banners

  1. Open an article in Content management
  2. Add &return=aW5kZXgucGhwP29wdGlvbj1jb21fYmFubmVycw== behind the URL
  3. Click Save & Close
  4. You are now on the Banner listing because that is the return URL used

Repeat step 1 & 2
3. Click Cancel
4. You are now on the Article listing instead of the Banner listing despite providing a return URL

Apply Patch

  1. Open an article in Content management
  2. Add &return=aW5kZXgucGhwP29wdGlvbj1jb21fYmFubmVycw== behind the URL
  3. Click Save & Close
  4. You are now on the Banner listing because that is the return URL used

Repeat step 1 & 2
3. Click Cancel
4. You are now on the Banner listing as expected due to the provided return URL

Expected result

After Cancel you end up where you started

Actual result

You end up at the Article listing

Documentation Changes Required

None

@ggppdk

This comment has been minimized.

Contributor

ggppdk commented Apr 11, 2018

Without testing

  • it looks good in case of cancel, we need to redirect back to the return url (after checking it)
  • but in case of save2new is it needed ? looking at the code (without testing) $this->getRedirectToListAppend() seems to already adding (passing) the return url,

also a little irrelevant but you would not verify it when just passing it, usually we would check it for being "internal" only before final usage ?

@roland-d roland-d changed the title from Add the return value to the Cancel and Save & New option to Add the return value to the Cancel option Apr 12, 2018

@roland-d

This comment has been minimized.

Contributor

roland-d commented Apr 12, 2018

@ggppdk Thank you for reviewing the code. You are correct, for Save & Close the change is not needed because the code is already in place there. I have updated the code and test instructions to reflect this.

also a little irrelevant but you would not verify it when just passing it, usually we would check it for being "internal" only before final usage ?

I am not sure what you are asking here. Are you talking about the isInternal check? That is just taken from how it is used in the Save action.

@ggppdk

This comment has been minimized.

Contributor

ggppdk commented Apr 12, 2018

I am not sure what you are asking here. Are you talking about the isInternal check? That is just taken from how it is used in the Save action.

I meant (for the code that you have now removed) when appending the '&return' variable
'&return=' . $someurl, no need to check that someurl is internal

$return_url = $this->getInput(...);

// No need check return_url is internal, just append it
// it will be checked when it is time to be used (redirecting to it)
$this->setRedirect($someurl . '&return = ' . $url);

Like you correctly check it in cancel,
because we have reached the point that it will be used (redirecting to it)

if (!is_null($return) && \JUri::isInternal(base64_decode($return)))
{
$url = base64_decode($return);
}

This comment has been minimized.

@ggppdk

ggppdk Apr 12, 2018

Contributor

Tested in backend (TODO some frontend form too) and it works,

but why not use an else statement here ?
and in it place the default url $url = \JRoute::_('index.php?option=' . $this->option .

Also inside the else instead of $url = \JRoute::_('index.php?option=' . $this->option . ...)
we should skip JRoute and just do $url ='index.php?option=' . $this->option . ...
because now we call JRoute 2 times ? (see setRedirect below) , and in frontend there might be a problem ?

This comment has been minimized.

@roland-d

roland-d Apr 12, 2018

Contributor

@ggppdk

but why not use an else statement here ?

Because an else is not required here and adds more code than needed. See more on Object Calisthenics and the else:
https://www.slideshare.net/rdohms/bettercode-phpbenelux212alternate/17-OC_2Do_not_use_the

Yes, the JRoute above can be removed.

I don't see an issue with the frontend as it this code already exists for the Save & Close. What issue do you see?

This comment has been minimized.

@ggppdk

ggppdk Apr 13, 2018

Contributor

About frontend i was thinking if the double call to JRoute::_ would be an issue now or in the future. It should be good now, i ll test

// Check if there is a return value
$return = $this->input->get('return', null, 'base64');
if (!is_null($return) && \JUri::isInternal(base64_decode($return)))

This comment has been minimized.

@joomdonation

joomdonation Apr 13, 2018

Contributor

Guess base64_decode($return) is not needed on this line because it is decoded in earlier command $return = $this->input->get('return', null, 'base64'); already?

This comment has been minimized.

@ggppdk

ggppdk Apr 13, 2018

Contributor

For a moment i had same thought,

but then i remembered $this->input->get('return', null, 'base64') is only filtering the characters allowed in a "base64" string,
it is not decoding the string (not calling base64_decode() on it)

https://github.com/joomla/joomla-cms/blob/staging/libraries/vendor/joomla/filter/src/InputFilter.php#L371-L388

Also the redirect to return url code, added by @roland-d

  • is identical to the code used inside the "default" redirect case of save() task

so the PR should be good now to test

There is 1 place inside the file
we do not need to check that it is internal
and also do not need to base64_decode it
that is inside getRedirectToListAppend() where we only want to pass it to the next page with &return=... instead of redirecting to it

@ggppdk

This comment has been minimized.

Contributor

ggppdk commented Apr 13, 2018

I have tested this item successfully on d9e3f67

Also i see that custom article controller for frontend
(and also some controllers of 3rd party components)

already support using return url for the cancel task
so this is change is consistent, all core and 3rd party components using the default controller should benefit for this


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20144.

@Quy

This comment has been minimized.

Contributor

Quy commented Apr 14, 2018

I have tested this item successfully on d9e3f67


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20144.

@Quy

This comment has been minimized.

Contributor

Quy commented Apr 14, 2018

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20144.

@joomla-cms-bot joomla-cms-bot added the RTC label Apr 14, 2018

@mbabker mbabker added this to the Joomla 3.9.0 milestone Apr 22, 2018

@mbabker mbabker changed the base branch from staging to 3.9-dev May 12, 2018

@mbabker mbabker modified the milestones: Joomla 3.10.0, Joomla 3.9.0 May 12, 2018

@mbabker mbabker merged commit 0968671 into joomla:3.9-dev May 12, 2018

5 checks passed

Hound No violations found. Woof!
JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@joomla-cms-bot joomla-cms-bot added PR-3.9-dev and removed RTC labels May 12, 2018

@roland-d roland-d deleted the roland-d:add-return-to-cancel branch May 25, 2018

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