I know it needs postgres and sql server changes to be made but I'll do that when this is all good and ready to go so if we change things around on review I have to change 2 files not 6 :P

Any test cases? ;)

This comment was created with the J!Tracker Application at http://issues.joomla.org/.

@wilsonge are you sure with the @SInCE tags like 1.0 or 1.6? I think we should use 3.4 for it?

Hullo - thanks for comments both. Sorry for lack of responses but had feedback elsewhere I needed to sort out first before testing.

@zero-24 changed the @since tags - you were quite right about that.
@b2z About to add testing instructions

Could you add _LABEL when it fits?
For example in :
COM_REDIRECT_FIELD_REDIRECT_STATUS_CODE

@test: The redirect works when setup. One thing I miss though is that the Apache log doesn't show the redirect. It only shows the 200 status for the target URL. I am not sure if that is an Apache setup issue or an issue with this code. From what I could find is that Apache logs 301 redirects out of the box.

If you choose a non-3xx code then it will display the generic error page in Joomla with the selected error code.

I cannot reproduce that, the page is still redirected to the destination URL. Like the 301 also a 500 error is not logged in the Apache log.

The selector is too small, make it wider for better readability.

Fixed the lang strings JM

@test: The redirect works as expected, it was a case of aggresive browser caching by FireFox. Picking another test URL and it works.

Only remaining issue is the width of the dropdown.

Thanks so much for the feedback Roland! Fixed the width of the select elements as requested :)

Having some issues with testing this patch on the latest development build 3.3.7-dev, when I try to force a 404 in order to redirect (rather than manually creating it from scratch) I'm unable to save it to the database.

The error message received:

1054 Unknown column 'header' in 'field list' SQL=SELECT `new_url`,`header`,`published` FROM `llr4k_redirect_links` WHERE `old_url` = 'http://local.joomlatestsites/j3/index.php/most-read-blah-2' LIMIT 0, 1 

Furthermore, the destination URL appears to be mandatory wheras it does not have an asterisk next to the label which, in my view as a user, would suggest that it was not a required field. Indeed with some error codes such as 410 (Gone) there is no need to redirect anywhere - it should just provide that error code as the page no longer exists.

@RCheesley To be able to save the change to the database you will need to manually execute the SQL file.

ALTER TABLE `#__redirect_links` ADD header smallint(3);

Thanks @roland-d - @wilsonge can you add that to testing instructions please?

Point still stands about the destination URL

@test successful with correct code being output in live headers, I have suggestions/comments for improvements

• When the code is different (e.g. 410) the page title & description remains the same (e.g. category not found) - should this be changed? Not found is appropriate for 404 error codes, but maybe not for other error codes.
• A destination URL is mandatory, however with some errors (e.g. 410 - Gone) no redirect occurs. While this should only be used by advanced SEO specialists, the fact you are required to specify a destination URL could be confusing. Perhaps where a status code is being used where no redirect would/should occur, the destination URL should be fixed as the original URL or be unavailable?
• From a UI perspective the location of the field is poor, because specifying the error code is at the last step. Perhaps we need to move it to the first step if you've got advanced turned on, or immediately below the original URL?

Updated description to add about the database change. Apologies for not making that clear.

The destination URL should not be mandatory in advanced mode will investigate that as that is a bug.

The location of the field is deliberate. Whilst there is only this 1 advanced field at the moment, the code has been written in such a way that infinitely more can be added by just adding them to the XML file. With that in mind it made sense for any of the admin fields to be added at the end at the current list. The other option was to create a new tab - but that made little sense for what is currently one field.

Thanks for the spot! They were written - just missed them out of the commit!

👍 The dates / names of the file gets fixed by the comitter with the commit date i guess. (3.4.0-2014-09-XX).

I would assume so too. The date is when I wrote the initial mysql file. I just kept the name consistent

Too late to get this in 3.4 instead of staging?

We are not going to have more 3.3.x series so I think it's ok 🍶

We are not going to have more 3.3.x series so I think it's ok

Famous last words? 😄

Then you guys get my awesome new feature early 😆

Hi!

I've found a B/C issue in 3.4.0-alpha related to a change done in /libraries/joomla/application/web.php, line 549

                if (!is_int($status) && !isset($this->responseMap[$status]))
                {
                    throw new \InvalidArgumentException('You have not supplied a valid HTTP 1.1 status code');
                }

If a third-party extension is still using $app->redirect($url, $msg) as $msg is not numeric, it will return a error 0 You have not supplied a valid HTTP 1.1 status code

PR with patch: #5247

@wilsonge could you confirm this is fixing the issue ?

Sorry @wilsonge to have disturbed you, i have close the PR because the issue was not in joomla core, but in a bad practice i've done a long time ago in one of my file, which one was working before 3.4.0-alpha... (in all cases, Thank you for your PR, because i was able to found a very crap code and to fix it in my own code! ) ;-)

Don't worry man. Glad you got it sorted :)

Warning!

See b979c6b#commitcomment-8793616

My suggestion is to fix the main B/C that I'm seeing with the old B/C API of Joomla 2.5 where second argument of redirect() is a string, and third too:

I commented here that the new statement at this line:
b979c6b#diff-ee867f6f28651cdefd87314f73708ecfR548

Suggesting that instead of is_bool($status) there we could have (! is_int($status)) to clean all non-int status codes, before checking for the fatal error thrown on non-int or illegal codes. That way, if it's not an int, it gets a valid redirect code, and if it's int, it still redirects new way.

Agree ?

@wilsonge To reproduce the B/C bug and test the proposed simple fix, just install Community Builder from web-installer, publish CB login module, and try logging in or logging out using it in frontend.

what when we used boolean for the 303/301 redirect?

@infograf768 https://github.com/joomla/joomla-cms/blob/staging/tests/unit/suites/libraries/cms/application/JApplicationCmsTest.php#L524 this unit test demonstrates your code should work perfectly still with a 301 redirect. If it is broken then it is not related to the changes I made.

@beat Thanks for code sample. I'll get a PR to fix stuff within the next couple of hours with unit tests

Beat your issue should be fixed by #5290

If we don't have a valid status code I added in your suggested check and just defaulted to 303 to keep b/c for now. However I think for 4.0 (when b/c can be broken) we still should revert to an invalid argument exception to keep things clean

I'd still advise you move to the non-2.5 way of doing things. Does passing in an empty message not just seem horrifically wrong xD

@wilsonge Thanks!

PR #5290 works for me too code-wise. Will test and feedback on the 5290 PR.

In Joomla 4.0, the whole B/C "if" part could be removed (which would have been another advantage of fixing the B/C issue in.... the B/C aera lol)

As said above, have added a task to modernize that part of old code :)

But I was worrying about existing sites and existing extensions (for 2 feedbacks here testing off master, probably 200+ would have broken too).

Btw, a redirect without a message to display on top is perfectly legit and ok imho in many situations where the hassle of a confirmation message for an obvious change of state is a UX overkill.

👍 Tested and reviewed PR #5290 looks good to me, which closes already the B/C issue raised above! Thanks @wilsonge for fixing it and @JoomliC for your report, feedbacks and tests too. 👍

i just wonder if @infograf768 has still language issue ?
b979c6b#commitcomment-8794568

Looks like I now get a 301 instead of the unwanted 303. Merging. Thanks.