Drupal 5.x support

I've made an attempt at updating the module to 5.x, but it causes Drupal to render blank pages. I've attached my patch file in case it's any help though.

subscribing...

subscribing

I've nearly completed the Drupal 5 migration of img_assist, but I didn't quite finish before I had to go out of town for the holidays. I'll be back next week and I'll probably finish this then.

Subscribing...

@benshell - can you post your progress as a patch so that we can all share in the progress?

I'm also updating the status.

I need this so urgently I could try to fix it tonight if someone will point me to the latest work in progress.

Suscribing too...
This module is just indispensable

Here's what I've done so far. I'm not ready to release this for 5.0, but if you have time you're welcome to help test this and fix it as necessary.

subscribing...
thanks for the latest. I'll give it a this evening.

I've started porting the module myself.

Now that I've found this, I'd like to help.

Any idea what's missing ?

subscribing

subscribing. (this module is the only thing holding me back from 5.0.)

subscribing..:-)

Has anyone made any updates since the prior version?

I've tested benshell's last "here's what I've got so far" almost-release on two sites with the same symptoms: when you click the img_attach button, the page that you're trying to insert into is itself reloaded to another page (although the image selector window DOES pop up.)

I'm really not up to writing code, but I can do testing... last time I look at img_assist's code I almost broke my mind :)

susbscribing

Subscribing...

susbscribing

This module is so useful that i keep coming back to check for any updates. I need it for my D5 site. Thanks for all the people that make it possible.

If we can raise $200, I've been told by one of the past maintainers of this module that they would find the time to update it to 5.0. Anyone interested in donating to a group effort to raise that amount of money?

Yes.

Adding to Boris:

yes, contact me at ethan [at] warnerbrosrecords.com

Subscribing...

subscribing

subscribing...

subscribe

unsubscribing... d'oh!

I'll contribute. martin.espinoza [at] gmail.com

RENAME FILE TO ZIP.

I've removed all deprecated use of theme_add_style() and used the drupal_add_css() instead.

I can get the img_assist module up and running, and there is an "add image" field below all my text areas. Unfortunately I can't get it to properly paste in the images :(

Furthermore, I tried copying the drupalimage tinymce plugin into the plugins folder of the tinymce engine in the tinymce plugin, but I can't find out where to administrate the buttons. The TinyMCE documentation mentions some configuration, but none of the files in the 2.0.09 package seems to have the init function mentioned.

Now I need help with getting these things to work, it seems to be javascript problems.

I'm not good with diff, so I'm attaching everything as an zip file. Hope someone can run with this.

To get drupalimage to work you need to do the following:

open modules/tinymce/themes/tinymce_full.js (or one of the others, depeding on what's included in your drupal page, check the source view of your browser)
add drupalimage to the array "plugins : " just follow the conventions there.
add drupalimage to one of the arrays theme_advanced_buttons2_add, just follow the conventions there. If you are in the other themes the arrays will be named differently. You can choose where to put it by mixing around the arrays.

This gets the "camera" icon on your toolbar, and pops up the img_assist window.

But still no joy in getting it to work :(

subscribing 2, I mean too. -- just cause it's in fashion...

Do not follow the install instructions for TinyMCE, it recommends you to use "sites/all/plugins" directory instead of the "plugins" directory. Install in the Plugins folder and then all the urls from img_assist will point to the proper index.php page.

Unfotunately it is still not working.

I'm concidering IMCE, but it is ugly, and works poorly and isn't suited as an replacement for the img_assist module.

Perhaps keep to 4.7.x is the solution I'll follow, at least I can get some sites finished there.

And is there anyone out there who are not only watching this thread but trying to find a solution as well?

subscribing as well...

I tested the file posted by pkej and it works well so far. (for my needs)
Thanks
jacauc

I can't get it to work at all, good job that it works for you at least :)

Well, I don't use the button to attach images.
I basically just need the input filter (so that I can upgrade my 4.7 site to 5) ...Images already attached.

I made an attempt to fix this and came up with this patch, based on some of pkey's work. It's working pretty well at least under some circumstances. I actually haven't tried it with TinyMCE yet, only as a standalone module, so it would be great if someone would like to test that. Another problem: it doesn't seem to work with the HTML format, only with filter tags.

These are the things I have worked on:

• Extracted two form builder functions from their meny callbacks. (I changed the indentation of a large block of code which makes the patch look larger than it actually is...)
• Changed references to form fields in javascript code from edit[xxxx] to edit-xxxx
• module_exist() >> module_exits()
• Added drupal_get_js() to theme_img_assist_page() – no javascript was being included!
• Proper use of drupal_add_css()
• The output of stylesheets in theme_img_assist_page() had to be reworked. I'm not sure about that part – there is some commented code and some unclear parts that someone with more experience with this module should take a look at. (Is it really a good idea to include theme style sheets in there? My theme's style.css pretty much breaks the layout.) I just tried to maintain the current solution, with the new API.

Testers and reviewers are much welcome!

I forgot to mention that the patch is against HEAD.

And here's an improved version. I think I understand the theming stuff a little better now. img_assist.css is supposed to load last which wasn't happening, so I fixed that.

I'm trying to focus on Drupal 5 compatibility only, but I couldn't help adding width: auto and max-width: 0 to body.img_assist in order to prevent themes from breaking the layout with those two properties.

For those who want to review the patch, the @@ -748,243 +760,252 @@ chunk is the part where the indentation was changed and therefore it's pretty hard to read. There are no real changes there though (apart from extracting the form stuff into form builders) so that chunk really doesn't need to be reviewed.

The HTML format still doesn't work. And it's still untested with TinyMCE.

Feels a little bit like I'm talking to myself here, but... I fixed another bug. Image uploads weren't working because of API changes. There is still a minor problem however. When you press submit, the form reappears with the image shown as a thumbnail but with the title missing. Re-entering the title and submitting again does the trick.

I could really use some help finishing this task. I know a lot of people need this module, and those people should include the previous maintainers and contributors to the module, right? Maybe the current state of the code is even good enough to be committed as a development version?

Yes, please create a development version. All I can help is testing. And I will...
And thanks for your work.

Hannes, would you be willing to maintain a Drupal 5 branch? I could talk to the current maintainer about making you a contributor. I'm not currently using this module in Drupal 5, so I can't afford to take the time to upgrade it.

Well I'm flattered that you ask. :)
First of all I am pretty unfamiliar with the design and the inner workings of this module. The work I've done has been in the form of trial-and-error and looking up lines of code that generate errors. Second, I'm afraid I won't have enough time to maintain this popular and somewhat complex module in a very responsible way.

If there is no one else who can do it, then sure, I could commit my changes in a Drupal 5 branch and maybe commit occasional patches, but I'm not sure I'll be able to maintain a proper stable version by myself. But hey, that's better than nothing I guess...

great job zoo33. thanks for your work. with the latest patch everything works as expected (except of the minor title problem - which includes also the image body content gets lost).

Thanks! That's great news.
There was one more thing that needed fixing though. The paths to the css files only worked on sites that use clean url's because I wrote url($path) when I should have used base_path() . $path. There was also a call to a deprecated function in the popup window code. So – here comes yet another patch!

Until there is a more permanent solution, I've made a tarball you can download and install if you want to help test it (or if you really need the module and you can live with the fact that it might be unstable).

Change the file extension from .txt to .tgz and unpack as you would any normal module. (File archives aren't allowed in issues anymore, but I think that in this case it's important that people get a chance to test it as soon as possible – please forgive me for this small violation of the Rules.)

Thanks for posting the bundle, but it's not working for me.

I get the link, the pop-up window, the image selection process, but when I click "Insert" I just get redirected to my main page (in the pop-up).

At least it's correctly displaying the images that were inserted before the upgraded, but my users aren't going to be happy if they can't add any new one....

Thanks again,
Maj

Tested the bundle. As with Majnoona, the pop-up is redirecting to /node/, but _only_ if I'm inserting the images as HTML Code. If I insert as Filter Tag, the image populates the node's text area just fine.

Also, still seeing the problem with the the thumbnail appearing when an image upload is submitted but having to re-enter title and body. On the second submit, everything goes through.

Lastly, using the Garland theme, the img_assist popup window's bottom frame overlaps the header frame, meaning you have to scroll to get the text and buttons. I took a look into this using Firebug, but didn't have any luck adjusting the frames. I have little experience using frames, though, so...

Haven't tested with Tinymce, due to the current instability of that module.

If time allows, I'll look into these matters and post any findings/fixes.

Michael Samuelson

Just a few observations from reading the patch (I have yet to test the functionality).

In the following chunk, why use function_exists('taxonomy_get_vocabularies')
You could use module_exists('taxonomy')), which is more standard to drupal development.

@@ -191,7 +200,7 @@
     '#options' => array('icon' => t('Show icon'), 'text' => t('Show text link'), 'none' => t('Do not show a link')),
     '#description' => t('Choose what to show under the textareas for which img_assist is enabled.'),
   );
-  if (module_exist('taxonomy')) {
+  if (function_exists('taxonomy_get_vocabularies')) {
     $vocs[0] = '<'. t('none') .'>';
     foreach (taxonomy_get_vocabularies() as $vid => $voc) {
       $vocs[$vid] = $voc->name;

Also, there is no Image package. That is reserved for multi-module packages. See the dev conference posts start around December 12, 2006 for more details. Basically, you should remove the package line. Also, add the following (include quotes), which will be replaced with the proper version by Drupal.org's package creator:

version = "$Name$"

+++ img_assist.info	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,5 @@
+; $Id$
+name = Img Assist
+description = This module allows users to upload and insert inline images into posts. It automatically generates an add image link under the textarea fields of your choice.
+dependencies = image
+package = Image

I only got about halfway through img_assist.module...

on line 574 of img_assist.module I found isJsEnabled(). It should be Drupal.jsEnabled as per http://drupal.org/node/64279

//		$output .= "  if (isJsEnabled()) { \n";
		$output .= "  if (Drupal.jsEnabled) { \n";

Line 575 - addLoadEvent has been replaced with jQuery's $(document).ready() as per http://drupal.org/node/64279

Unfortunately, neither of these corrections resolve the issues noted above.

Michael Samuelson

Yes, finally some communication! :)

@majnoona & mlsamuelson:
Yes, that bug has been there all along. I don't know how to fix it, but it's not really a problem for my own needs, so if someone else wants to take a crack at it, go ahead!

@mlsamuelson:
Please look at the upload form issue if you have time. I nailed the problem with Garland: .form-button had too much vertical margin... (Add margin: 3px; to .img-assist .form-button {} to fix that.)

@moonray:
Thanks for your review! I agree about module_exists – that change wasn't introduced by me. I changed that in this latest version.

I've also followed your advice with regards to the .info file. There is actually an Image package –image.module comes bundled with two modules which forms the Image package – but that doesn't mean img_assist should be a part of that package. (However I've seen several modules use the package info more freely than what is agreed, for example the "Input formats" package...)

@mlsamuelson
I didn't see your last post before I posted mine...

On line 575, can we simply put $(document).ready(parent.initUpload) or do we have to do something differently?

It would help to maintain quality in all branches of this module if we limit our reviews to changes that were introduced by this patch and address other problems in separate issues.

Hannes, Ben has given you CVS access to the project. Let me know if you need help creating a Drupal 5 branch.

I guess I should first update HEAD with this patch and then branch it – right?

Yes.

@@ -748,243 +760,252 @@

This block is purely whitespace modification, right? Can this be applied to CVS separately, and be left out of the patch so that review can be more easily done? :-)

In the following chunk, should the word assist be lowercase (Drupal 5 standard is Sentence capitalization)? It is the module's name, though...

@@ -67,10 +67,19 @@
[...]
+      'title' => t('Image Assist'),
[...]

Line 181, shouldn't that be t('Display Image Assist'), or t('Display Image assist'), instead (whichever is the standard)?

    '#title' => t('Display img_assist'),

Line 212. t('Select the vocabularies to use for Image Assist'),

        '#title' => t('Select the vocabularies to use for img_assist'),

etc. Basically, replace every instance of "img_assist" in forms, menus, and in text on page to "Image Assist" (or "Image assist"). And, if we refer to it as "Image Assist" and not "Img Assist", shouldn't the module's name in the .info file be adjusted as well? Either way, it should be consistent.

Then, a few more Sentence capitalization inconsistencies:

Line 198 would become '#title' => t('Textarea image link'),.

    '#title' => t('Textarea Image link'),

Line 961 should be '#title' => t('Insert mode'),

      '#title' => t('Insert Mode'),

Now for some layout:
The buttons in the header frame appear too low (unreadable) in Firefox. To fix this, add margin: 0; to the body.

Line 19.

body#img_assist_header {
  background-color:#ccc;
  margin: 0;
}

The following code will align all the buttons properly. Add it around line 99.

#header-uploading input, #header-properties input, #header-browse input, #header-startover input, #header-cancel input, #header-uploading select, #header-properties select, #header-browse select {
  margin-top: 0;
  margin-bottom: 0;
  vertical-align: baseline;
}

subscribing. Is there a tar/zip version of this that can be downloaded with all the patches? I am interested in testing once I move other stuff into my 5.0 site, but I am a little confused about where to get the package.
Thanks,
Maria

@moonray
We should try to keep to Drupal 5 compatibility only with this patch. I know I have already sneaked in a few other small changes, and I have now followed your advice and changed the UI text to use "Image assist" consistantly (it should be "Module name", not "Module Name", according to this). We should keep track of these changes so that some of them can be ported to the 4.7 branch.

However I think we should wait with the CSS changes and put those in a separate issue/patch. (Also, it's working fine in Firefox for me, but that actually depends on which theme you use. All that makes me wonder if we should even include any style sheets at all, other than tinymce.css and img_assist.css. But again, let's save it for another issue.)

I also reverted the whitespace so that the patch is easier to read (= possible to read). I'll make another patch which corrects this as soon as this one is done.

Now, are we ready to commit this? There are two known bugs (HTML format not working and disappearing title/body when uploading images) but those can be filed as separate issues. I'll check back in a couple of hours/tomorrow and see if there are any more proposed changes – if not, it's going in!

@mariagwyn
Check comment #45. That version is working pretty well, but an improved version should be available very soon so you might want to wait.

We should all thank Hannes (zoo33) for his hard work getting a Drupal 5 version ready.

Agreed. Looks good. Commit. :-)

I'll post a new issue about the CSS issue.

Thanks for those kind words!
I just committed this to HEAD and made a Drupal 5 branch.

For the remaining bugs I've created these issues: 118829 and 118831

There is also moonray's CSS issue: 118790

The following changes from this patch could be worth porting to 4.7:

• t('Image Assist Propperties') should be t('Image Assist Properties').
• Add width: auto; and min-width: 0; to body.img_assist in img_assist.css in order to keep theme css from breaking the layout.
• Anything else?

I'll leave that for someone else to look at... :)

Definitely a big thank you to Hannes!

Wish I could have helped out more yesterday, but my Internet provider was down all day. Painful.

I'll check out the new issues and see if I can't uncover anything.

mlsamuelson

Thank you very much, Hannes. I've downloaded and starting using the D5 branch. I have not seen any serious problems so far.

Seems like there is no interest in back-porting that stuff.