I wanted to work on a patch for globalredirect (#201675: Redirect to version in native language), but soon found myself doing code cleanup, mostly to get my own head clean.
I thought I should propose the changes as a patch, before I add any new features.

Changes:
- Much shorter syntax for everything related to settings variables.
- Only one place (instead of two) to define settings default values.
- To make globalredirect_init shorter, initial checks if globalredirect should run at all are moved out of hook_init, and into _globalredirect_should_run(). Some of the if conditions are split up into digestible pieces.

There are probably more things that could be refactored, but I wanted to keep the patch small.

The intention is not to win a beauty price, but to get a tidy and manageable base to start with new features, or to adapt in a new module (#775748: Define a hook for other modules).

Patch is against 6.x-dev.

CommentFileSizeAuthor
#18 globalredirect.3.patch21.62 KBdonquixote
#9 globalredirect.everything.2.patch20.85 KBdonquixote
#5 globalredirect.everything.patch20.56 KBdonquixote
#4 globalredirect.readability.patch9.36 KBdonquixote
globalredirect.readability.patch9.34 KBdonquixote

Comments

YK85’s picture

YK85 commented

subscribing

nicholasthompson’s picture

nicholasthompson
Primary language English
commented
Status: Active » Needs work

Looks good - a few minor "coder" level pointers that I can see by visually parsing it... Boolean values should be in capitals (IIRC)...

I like the idea of moving the settings into a function though - is it worth chucking some static caching into it?

donquixote’s picture

donquixote commented

static caching
-> The function should not run more than 1x per request. In rare cases it can run twice (so I read in the comments). Static caching always requires a bit extra thinking, to make sure there are no side effects. In this case, I would say no.
-> In the future we could combine the settings in one single variable (array-valued), instead of those separate variables. This will require a bit of hook_update, and I didn't want to do that in this patch.

TRUE/FALSE instead of true/false
-> oui mr.

Any other coder issues?
Do I need to re-roll the patch, or will you just do it yourself and commit?

donquixote’s picture

donquixote commented
Status: Needs work » Needs review
StatusFileSize
new globalredirect.readability.patch9.36 KB

again..

donquixote’s picture

donquixote commented
StatusFileSize
new globalredirect.everything.patch20.56 KB

Now it's all in:
- heavy refactoring, everything split up in little functions.
- a hook_globalredirect_destination_alter(&$langcode, &$system_path) for other redirect modules.
- Two options for mixed language node redirects (translation and native language).
- Attempt of full backwards compatibility. The code is intended to be equivalent to the old version, if (i) the mixed language redirect setting is disabled, (ii) no other modules implement hook_globalredirect_destination_alter, (iii) the code is bug-free (which we'll have to find out).

Happy testing, everyone..

okokokok’s picture

okokokok commented

The refactoring into functions makes the code much more readable.

Plus it's working properly on one site I've tested. Will test other sites later.

finex’s picture

finex commented

subscribing... I will test this patch soon

donquixote’s picture

donquixote commented

Just a note:
If you want a patch that only does refactoring, without the new features, all you need is remove the call to _globalredirect_alter_destination($settings, $request, $options); (and then remove the functions that are never called).

donquixote’s picture

donquixote commented
StatusFileSize
new globalredirect.everything.2.patch20.85 KB

The previous patch only worked for path prefix. This one should also work for language selection based on domain name.

nicholasthompson’s picture

nicholasthompson
Primary language English
commented

I'm very keen to get this one committed into GR - I'm writing the test at the moment and SERIOUSLY regretting those stupid defined setting names!

nicholasthompson’s picture

nicholasthompson
Primary language English
commented

For reference, I just closed #374112: Defines and checkboxes as duplicate of this.

donquixote’s picture

donquixote commented

Ok, but if you actually want to abolish the defines, you need a second step after this patch.
Same for the checkboxes.

I think it's a good idea to do this one after the other, and first get this patch through. It will be a lot easier to get rid of the defines, as it will only affect a small function (plus the admin form, of course).

I also think it could be a good thing to have all values in one array-valued variable, instead of separate ones. It is a rare case that you need the value separately.

For things with three options, you can use boolean false or NULL for "disabled" (that's nice in if statements), and string values for the different "on" states. I don't see the benefit of define.

Checkboxes are great for things where the "disabled" state does not need further explanation. Otherwise, a pair of radios can be very useful to explain what happens if you choose "disabled".

YK85’s picture

YK85 commented

The patch at #9 doesn't seem to apply cleanly.

patching file globalredirect.admin.inc
patching file globalredirect.module
Hunk #4 FAILED at 80.
Hunk #5 FAILED at 118.
2 out of 5 hunks FAILED -- saving rejects to file globalredirect.module.rej

I'm excited to try the improvements of your hard work!
Thanks

donquixote’s picture

donquixote commented

did you use the dev version? which one?

YK85’s picture

YK85 commented

Global Redirect 6.x-1.x-dev (2010-May-29)
Thanks!

robby.smith’s picture

robby.smith commented

subscribing

donquixote’s picture

donquixote commented
Title: A bit of refactoring / code readability » A bit of refactoring + mixed language redirects

I think this issue title is more descriptive.

donquixote’s picture

donquixote commented
StatusFileSize
new globalredirect.3.patch21.62 KB

@yaz085 (#15):
still having this problem?
I did the patch again, don't know if it helps.

nicholasthompson’s picture

nicholasthompson
Primary language English
commented

Hi guys - I've applied some of the readability work to the module in DRUPAL-6--1 however some I haven't... I'm not convinced that breaking the function into sub-functions makes it more readable as it causes the reader to have to hop around the code to follow the flow.

I have, however, used the "globalredirect_is_active" idea (with some modifications) - very clean idea to wrap that into a function.

There is also a two-phase simpletest bundled with it now which tests several features (in both enabled and disabled mode). I need to add more tests to this still, but its a start.

I still need to review the mixed language side of this patch.

donquixote’s picture

donquixote commented

I'm not convinced that breaking the function into sub-functions makes it more readable as it causes the reader to have to hop around the code to follow the flow.

For my personal taste, I strongly prefer a bunch of small functions to a long heap of rather unstructured code. But this is just one part of the story.

The more important point of separate functions is to reduce the scope of variables. Where possible, I prefer variables that only live within a small function.

The way I split this thing up is probably not the perfect and final solution, but it is a step forward, and it should be easy to continue from there.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

Side note the latest commit with http://drupal.org/cvs?commit=380028 will not work with PHP4 (the foreach/reference syntax of foreach ($items as $key => &$value) {. You have to modify using $items[$key] = $newvalue.

nicholasthompson’s picture

nicholasthompson
Primary language English
commented

Bugger - thanks for spotting that.

Does D6 even support PHP4?

nicholasthompson’s picture

nicholasthompson
Primary language English
commented

I've removed the references to &$value... Cheers for that Dave

jcmarco’s picture

jcmarco commented

In last commit, at .install, there is this change:

46 $value = 0; $ettings[$key] = 0;

Is that right? it seems that it should be $settings.

iztok’s picture

iztok commented

Would this module redirect site editors to node's real path after submiting that node?

For example I add a node on /fr/node/add/page with language set to English. Now user would get to a /en/node/1 page but I would like to be redirected to its real path /fr/node-alias. Website editors only use Fr for admin!

Also which version would I need to use and/or patch to try this?

And thank you for your work! I have been loosing my mind now on how deal with translations by not forcing users to use administration (adding and editing nodes) in more than one language.

nicholasthompson’s picture

nicholasthompson
Primary language English
commented

jcmarco: That was a typo - it's been fixed/

Iztok - I have committed a patch from #201675: Redirect to version in native language which seems to do this - however testing is needed.

donquixote’s picture

donquixote commented

I have worked a bit more on the refactored version, and it behaves nicely on a site of mine for a few months now.
I wonder if I should publish this thing as a separate module, as a "proof of concept" and "code takeaway", or, if people like it, as a real alternative.
This way we get the new stuff, but no risk for existing sites that use globalredirect.

I noticed that Dave Reid already started his own thing,
http://drupal.org/project/redirect
Does this have the same purpose, or something different?