QFaN: how thorough are redirects supposed to be?

[ninetydegrees]

The case: bug #2398 is about moving editpics to editicons. If I have a look at the patch submitted for a similar bug (bug #2396) it seems simple enough (and something a baby like me managed to actually patch \o/).

The question: should the 'renaming' be more thorough for this bug? In other words, should all references to editpics in other files such as allpics.bml and /site/index.bml be changed and should related files such as editpics.css and editpics.js be 'renamed' too? Or should we trust the redirect to make everything work smoothly and not bother?

Comments

[mark]

When renaming something like that, it should include all URLs that link to it. So, menus, links, etc should all be updated.

The CSS and JS files can remain as editpics, although it doesn't hurt anything to rename them if you want.

[ninetydegrees]

So thorough is good. Thanks!
If you don't mind, I have a few more questions as there are some things left I'm unsure about.

I get 'editpics' in files I have no idea what they're really for:

cgi-bin/Apache/BML.pm
cvs/bml/lib/Apache/BML.pm
cvs/bml/test/fake_root/bml-test.correct
cvs/bml/test/fake_root/bml-test.bml
cvs/bml/test/fake_root/global.look

Should they be patched too?

In editpics.js there is a function called editpicsInit. Should it be renamed?
In editpics.css there is a class called EditPicsUserpic. Same question. Both are used in editpics.bml and only there if I'm not mistaken.

In editpics.bml, there are also a few lines I'm unsure about:

            if ($inline .= LJ::Hooks::run_hook("cprod_inline", $u, 'EditPics')) {
                    if ($inline .= LJ::Hooks::run_hook("cprod_inline", $u, 'EditPicsMax')) {

Should I change these?

In en.dat, there are some strings which use editpics in their names. Most of them seem unused but this one is:

cprod.editpics.text7.v1=You've reached your limit of [[num]] userpics.

Should they be renamed as well?

[afuna]

Hm, okay to clarify, "cvs" stands for "concurrent version system", which is to say, it's what keep track of the code that has changed, and is also what is used to synchronize changes among developers. So anything under cvs/ is the copy of the code that is used to coordinate code between developers. Anything not under cvs/ is your actual working site. You can find an equivalent for all files between your actual site and the code under cvs.

So:
cgi-bin/Apache/BML.pm
cvs/bml/lib/Apache/BML.pm

Are basically the same file, and you'd only need to edit one of them when making your patch -- in this case, cgi-bin/Apache/BML.pm.

Anyway, the mention of editpics in cgi-bin/Apache/BML.pm looks like an explanatory comment; yup edit it.

cvs/bml/lib/Apache/BML.pm - mirror of cgi-bin/Apache/BML.pm, so the patch you make will be affecting both. No need to edit this
cvs/bml/test/fake_root/bml-test.correct - these three files are used to test whether BML is working correctly, but they're more sample text than anything else. They're not used on the actual site, so it's not critical that they match wrt editpics versus editicons. No need to edit.
cvs/bml/test/fake_root/bml-test.bml
cvs/bml/test/fake_root/global.look



editpics.js and editpics.css -- hmmmm. no harm done if they're not renamed, since CSS classnames and function names are not user-facing, so you don't *need* to rename them, but I say sure, go ahead and rename.


en.dat -- nah, no need to rename.



if ($inline .= LJ::Hooks::run_hook("cprod_inline", $u, 'EditPics')) {
if ($inline .= LJ::Hooks::run_hook("cprod_inline", $u, 'EditPicsMax')) {

-- hmmmm. No, don't edit.

Quick explanation: hooks are basically a way to implement site-specific logic. Those lines above basically mean to try to do any site-specific logic which was implemented for an inline cprod identified by "EditPics" (and "EditPicsMax"). We haven't implemented any additional logic for cprods, so changing them won't affect us. However, we can't guarantee that other sites using DW code have done the same, so to be safe, don't change them.

[triadruid]

Would it make sense to add a second hook under the new title, or at least a comment line with the new keyword? I expect institutional memory will eventually fade, and someone will forget to grep for both 'editpics' AND 'editicons'...

[ninetydegrees]

Thanks for your great answer. One more question: how do I include cgi-bin/Apache/BML.pm into my patch? It's neither in dw-free nor dw-nonfree.

Also, I'm unsure about forms like these:

$body .= "<form method='get' id='userpic_authas' action='editpics$suffix'>\n";

[afuna]

Oops, you're working directly in dw-free/dw-nonfree, I didn't realize! In that case, edit the version of the file under cvs/bml/lib/Apache/BML.pm, then.

Hmmmmmmmm. Yes you can fix those as well, just make sure that they're also fixed in whatever processes the form (likely something that looks for $POST{editpics$suffix} somewhere down the page)

[afuna]

Hmmm, thinking, thinking. I'm worried that it will complicate things. It may be easier to go through the hooks at some later point and rename them so they make sense (or document the hell out of them), so any announcements of incompatibilities could be done in one go, rather than piecemeal.

[ninetydegrees]

Ok I'm feeling dumb now. In dw-free, cvs/bml/lib/Apache/BML.pm doesn't exist. I only have dw-free/cvs/multiccvs.pl.

[afuna]

Ah! Sorry, no, that's my fault for not being clear. Go to $LJHOME/cvs/bml/lib/Apache/BML.pm

($LJHOME/cvs contains dw-free, dw-nonfree, and a bunch of other folders, including bml)

[ninetydegrees]

Ah my bad! I knew that but couldn't figure out to make a patch: I kept trying to create a Mercurial repo in the wrong folder (cvs instead of bml because I didn't see .hg was there).

[ninetydegrees]

Yes you can fix those as well, just make sure that they're also fixed in whatever processes the form (likely something that looks for $POST{editpics$suffix} somewhere down the page)

Hmm. I don't see anything like this, no.

[afuna]

Durrrr. My bad again. I was skimming and I thought it was something like <input ... >, and instead it's the page that the form is supposed to post to! So um, yeah, heh. Just change those to editicons.

[ninetydegrees]

Well, I have that one too:

<input type="hidden" id="go_to" name="go_to" value="editicons$suffix$getextra" />

[afuna]

Hmmm, I'm not absolutely certain what that is for, but it looks like a reference pointing back to the current page. That is to say: so yeah, edit it.

[ninetydegrees]

Ok so I had everything edited in this file. I'm going to regenerate the patch though since some bml files have been modified since. Thanks for going through this with me with your usual patience and graciousness.

[afuna]

It was my pleasure :)

*salutes*