Skip to content
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

WIP Update Router.php #1541

Closed
wants to merge 1 commit into from
Closed

Conversation

sugenganthos
Copy link
Contributor

@sugenganthos sugenganthos commented Nov 26, 2018

Example my routes def:
`
$routes->get('/search', 'Home::search'); // def 1

$routes->get('/(:segment)/(:segment)/{locale}','Product::details/$1/$2'); // def 2

$routes->get('/(:segment)/(:segment)','Product::details/$1/$2'); // def 3

$routes->get('/(:segment)','Product::category/$1'); // def 4
`

My url:
uri 1 : http://mysite.test/clothes should go to Product::category('clothes')
uri 2 : http://mysite.test/clothes/tshirt should go to Product::details('clothes','tshirt')
uri 3 : http://mysite.test/clothes/tshirt/id should go to Product::details('clothes','tshirt') with indonesian locale

Error found, it says "undefined offset"

First bug:
After evaluating def 2 , $localeSegment has array index. Then evaluating def 3. That value is retained and causes out-of-range index of $this->detectedLocale = $temp[$localeSegment];
So $localeSegment needs to be reset every iteration. I don't know if this a bug or intended.

Second bug:
I separate $localeSegment = array_search('{locale}', explode('/', $key)); just for debugging purpose.
Becomes:

$exp = explode('/', $key); $localeSegment = array_search('{locale}', $exp);

When evaluating def 2 which is '/(:segment)/(:segment)/{locale}'
Turns out the $exp = [ "([^" , "]+)" , "([^" , "]+)" , "{locale}"] (5 elements) instead of $exp = [ "([^/]+)" , "([^/]+)" , "{locale}"] (3 elements)
So it has error because '/' in regex takes into account for explode() function.

$temp = (explode('/', $uri)); $this->detectedLocale = $temp[$localeSegment];

On uri 3,
Exploding $uri results 3 elements, but exploding matched $key results 5 elements --> "undefined offset"

I see 3 possible solution for this:

First, my used solution,
replacing regex temporary with 'x' just to remove '/' before exploding. I think this is not good solution because to many step ( placeholder -> regex -> dummy string -> regex again). But it works for me until now.

Second,
Exploding and finding {locale} index is done before replacing placeholder with regex. So '/' will not mess with explode(). I don't know how to implement this. CI4 is a large framework.

Third,
using preg_split instead of explode to split keys only with '/' outside parenthesis. So key "([^/]+)/([^/]+)/{locale}" will results [ "([^/]+)" , "([^/]+)" , "{locale}" ]

// Are we dealing with a locale?
if (strpos($key, '{locale}') !== false)
{
$localeSegment = array_search('{locale}', explode('/', $key));
$tempkey = preg_replace( '#\(([^()]*)\)#i' , 'x' , $key);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

temporary replacing regex with 'x' just to remove '/' character

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might work for your usecase, but dpesn't feel right for the general usecase. I agree that your other two proposed solutions would be better. The second suggestion, handling locale separately form the rest of the URI, sounds most promising.

Would another workaround be to put the locale in the second segment if it is to be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is good to place locale there. But some user just remove locale segment (they rely on website's link to change locale instead on typing on the address bar) and website rely on default locale.

So my usecase is put locale segment on last segment

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the routing needs to handle the special case the the locale might be the last segment instead of the first one then? Actually, it needs to woek either way.
Hmm - this might provide the basis for a unit test to expose the problem :)
Either way, simply replacing {locale} with x doesn't seem right :-/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure it not right :( , it is just quick hack. In the end, I just use GET variable to change locale and save it to visitor's cookie. My routing has assign 0 - 3 segment already, so locale segment can misinterpret as path segment.
If locale always shown on 4th segment, that will be easier todo. Unfortunately, path with 2 segment can also have {locale} and misinterpret as 3rd segment
This is the cost to make url looks memorable :)

In the end, I just remove routing with {locale} and change locale using GET instead and save it to visitor's cookie

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway, if {locale} segment gets overlap with path / parameter segment, that is developer problems, nothing todo with framework. Naturally it will get misinterpret, which comes first.
{locale} may misinterpret as another regular segment.
or ordinary segment misinterpret as locale code (surely will fallback to default locale, but the regular segment won't get executed)

@jim-parry jim-parry added the bug Verified issues on the current code behavior or pull requests that will fix them label Nov 28, 2018
@jim-parry jim-parry added this to the 4.0.0 milestone Nov 28, 2018
@jim-parry jim-parry added in progress and removed bug Verified issues on the current code behavior or pull requests that will fix them in progress labels Nov 28, 2018
@jim-parry jim-parry changed the title Update Router.php WIP Update Router.php Nov 28, 2018
@atishhamte atishhamte mentioned this pull request Mar 27, 2019
3 tasks
lonnieezell added a commit that referenced this pull request Mar 29, 2019
@atishhamte
Copy link
Contributor

@lonnieezell, its done. Could you please close this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants