We so need a 'save draft' feature :|

Patch details:

  • fapi conversion.
  • Added status messages, comments and language fixes in places.
  • Added field sets to certain forms.
  • Removed #return_value for checkboxes and radios as the former triggered a fatal error. The default is apparenly 1, which was the value assigned anyhow.
  • Translate string form: added a negative weight to the original string.
  • drupal_goto -> return

Notes:
-The search string form should probably use sessions to store search criteria. It currently scans $_REQUEST.
-Delete strings possibly need a confirmation form.

Please review,
Thanks
-K

CommentFileSizeAuthor
#10 52911_locale.fapi.patch41.06 KBZen
#3 locale.diff124.21 KBZen
#1 locale2_0.patch41.09 KBZen
locale2.patch41.11 KBZen

Comments

Zen’s picture

Zen commented
StatusFileSize
new locale2_0.patch41.09 KB

Fixed a couple of erroneous indentations.

-K

killes@www.drop.org’s picture

killes@www.drop.org commented
Status: Needs review » Needs work

please don't intermingle bug fixes with reformatting code or fiying typos, this makes it hard to see what the real issue is.

Zen’s picture

Zen commented
Status: Needs work » Needs review
StatusFileSize
new locale.diff124.21 KB

Point taken. Please use attached diff to compare superficial changes.

Thanks
-K

killes@www.drop.org’s picture

killes@www.drop.org commented
Status: Needs review » Needs work

Sorry, no.

I want a real diff without the formatting and the typo fixes.

Zen’s picture

Zen commented
Status: Needs work » Needs review

Point taken and I agree. But, please make an exception here, as rolling this again is going to be a chore. If you still don't prefer to review this, please consider leaving this up for review and perhaps somebody else will.

I'll make sure that I split the formatting into a separate patch in the future.. I just can't work with unformatted code and can't resist fixing this stuff, however much I try to restrain myself.

Thanks :)
-K

dries’s picture

dries commented

I spent 10 minutes reviewing the code, and it looks good to me.
I'd like to see this committed too.

killes@www.drop.org’s picture

killes@www.drop.org commented

I'd like to see it committed too. Has anybody tested it?

dries’s picture

dries commented

I haven't tested this one; just spent some time looking at the changes.
If someone can spent 10 minutes testing it, it should be ready to go in.

riccardoR’s picture

riccardoR commented

I applayed locale2_0.patch to current CVS and I'm doing some testing. (@Zen: great work!)
I got an empty page (no errors displayed or logged) whenever trying to edit a string translation (?q=admin/locale/string/edit/nnn).
As the original patch is quite long I print my fix here, so it is easy to review.

@@ -407,7 +407,7 @@ function locale_string_search() {
  */
 function locale_admin_string_edit($lid) {
   include_once './includes/locale.inc';
-  _locale_string_edit($lid);
+  return _locale_string_edit($lid);
 }

 /**

Thanks and have the best,
Riccardo

Zen’s picture

Zen commented
StatusFileSize
new 52911_locale.fapi.patch41.06 KB

Thanks for the review Riccardo :) I've fixed that.

I also noticed in the resulting function that there was some hocus pocus with substituting textareas with textfields based on the string length (40). This will cause data loss/corruption for strings below 40 characters (which is very possible) that have line breaks in them. Moreover, the recent patch to form.inc strips \r, \n from all textfields (not textareas). I've switched this to resize the number of rows based on the word _count_ [12], with a maximum of 10 rows. str_word_count is compatible with PHP >= 4.3.0.

Please review again - much appreciated :)

Cheers,
-K

Zen’s picture

Zen commented
Status: Needs review » Reviewed & tested by the community

Patch still applies cleanly. Affected areas - all admin forms.

Testing my own patch:

  • Adding a language via language list - OK.
  • Adding a language via custom language form - OK.
  • Enabling/disabling a language - OK.
  • Setting/unsetting a language as default - OK.
  • Deleting a language - OK.
  • Imported a .po file - large spanish translation file - OK.
  • Exported Spanish to a .po file - OK.
  • String search - OK.
  • Edit and save a string - OK.
  • String edit forms - auto-resize - OK.
  • Delete a string - OK. [No confirmation appears as mentioned above.]

That's about it.

Setting to RTC.

Cheers
-K

killes@www.drop.org’s picture

killes@www.drop.org commented
Status: Reviewed & tested by the community » Fixed

applied.

Zen’s picture

Zen commented
Status: Fixed » Closed (fixed)