? Pending

User tests: Successful: Unsuccessful:

avatar brianteeman
brianteeman
21 Aug 2017

PR for #17377

There is a comment in the original code that makes zero sense and the code with it is the cause odf the problem. This PR removes that code

Before this PR if you create a bulk import it inserts the site name see video
bulk

avatar joomla-cms-bot joomla-cms-bot - change - 21 Aug 2017
Category Administration com_redirect
avatar brianteeman brianteeman - open - 21 Aug 2017
avatar brianteeman brianteeman - change - 21 Aug 2017
Status New Pending
avatar tonypartridge
tonypartridge - comment - 22 Aug 2017

Actually to me it makes sense, since it only inserts the url if the it doesn't contain the root url and the redirects component can only act on the root url.

Doing what you are doing actually will break the importer for anyone who is just inserting the relative path already.

I think we just need to add the same method to the new url, which is actually just a http/https test.

The issue with @coolcat-creations is she is inserting a / before, to solve her issue I would just use:

  •   		$old_url = JUri::root() . $batch_url[0];		
    

to

  •   		$old_url = str_replace(JUri::root() . '/', JUri::root(), JUri::root() . $batch_url[0]);		
    

Will fix her issue, but we also have bigger issues like, bulk import allows importing already existing urls and invalid redirect new urls.

avatar tonypartridge
tonypartridge - comment - 22 Aug 2017

@brianteeman Can you try replacing the section of code within the loop and before the query with:

$root = JUri::root();
			// Source URLs need to have the correct URL format to work properly
			if (strpos($batch_url[0], $root) === false)
			{
				$old_url = str_replace($root . '/', $root, $root . $batch_url[0]);		
			}
			else
			{
				$old_url = $batch_url[0];
			}

			// Destination URL can also be an external URL
			if (!empty($batch_url[1]) && strpos('http', $batch_url[1]) !== false)
			{
				$new_url = $batch_url[1];
			}
			elseif (!empty($batch_url[1]))
			{
				$new_url = str_replace($root . '/', $root, $root . $batch_url[1]);
			} 
			else {
				$new_url = '';
			}

this should insert the urls correctly and remove the double slash.

avatar brianteeman
brianteeman - comment - 22 Aug 2017

@tonypartridge the issue is that it is different in the bulk import to the manual entry

avatar tonypartridge
tonypartridge - comment - 22 Aug 2017

@brianteeman it is, we also need to fix that. since we cannot use relative urls in Joomla! com_redirects it has to be absolute. Relative links do not work. I'm looking at that now, ideally relative links would be better.

avatar tonypartridge
tonypartridge - comment - 22 Aug 2017

@brianteeman ok I'm going to do a bigger pull shortly which some good improvements to the com_redirects.

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 22 Aug 2017

@tonypartridge are you aware of #17653?

avatar tonypartridge
tonypartridge - comment - 22 Aug 2017

@franz-wohlkoenig yep I already commented on it ;-)

avatar brianteeman
brianteeman - comment - 22 Aug 2017

@tonypartridge it was an attempt at a quick fix to make the behaviour on inserting a record the same when you do a bulk import as a manual import

avatar tonypartridge
tonypartridge - comment - 22 Aug 2017

Yep, but actually it's broken. Inserting as relative isn't support by
com_redirects for installations in a sub folder hence a bigger pull is
needed

--
Tony Partridge

From: Brian Teeman notifications@github.com notifications@github.com
Reply: joomla/joomla-cms
reply@reply.github.com
reply@reply.github.com
Date: 22 August 2017 at 07:46:06
To: joomla/joomla-cms joomla-cms@noreply.github.com
joomla-cms@noreply.github.com
CC: Tony Partridge admin@xtech.im admin@xtech.im, Mention
mention@noreply.github.com mention@noreply.github.com
Subject: Re: [joomla/joomla-cms] [com_redirect] Bulk import (#17658)

@tonypartridge https://github.com/tonypartridge it was an attempt at a

quick fix to make the behaviour on inserting a record the same when you do
a bulk import as a manual import


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#17658 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABVglmU2mCNaSbyJRa6YnUNFZInrV3zgks5sankugaJpZM4O9lPt
.

avatar brianteeman
brianteeman - comment - 22 Aug 2017

OK well I will close this then and await your PR instead.

avatar brianteeman brianteeman - change - 22 Aug 2017
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2017-08-22 06:48:58
Closed_By brianteeman
Labels Added: ?
avatar brianteeman brianteeman - close - 22 Aug 2017

Add a Comment

Login with GitHub to post a comment