Port to 6.x

While I'd bet that we'll release 5.x-2.0 earlier than 6.x-dev, I'd appreciate any patches.

Subscribing.

subscribe

subscribe

This would be great to have with 6.0. Subscribing.

Please.... My site looks horrible without images...

Seems like there's plenty of interest in here... Porting Image Assist for D6 will probably take between one to two days. I'll probably not work on a D6 port of Image Assist unless I'll need it myself for a new project. If it's needed earlier -- would anyone want to chip in for the port?

Chipping EUR 25

subscribe

subscribe

I attached a patch for img_assist-5.x-1.x-dev (2008/02/24). Showing existing images is ok, It's not possible to insert new images yet with the img_assist popup panel but you can insert tags by hand. I'm not a coder, so it was more trial and error and much reading on drupal.org.

Maybe it's a beginning.

Don't forget to change img_assist.info (dependencies - is an array now, core).

I moved the CSS tags into my v6 theme so that my existing blog entries will display correctly. I'm hand editing new blog entries, using the same Image Assist tags, although it isn't very handy! Add me to the list of people who are eagerly anticipating a v6 compatible release. Thanks!

New patch:

- inserting tags for existing images is now possible
- upload of new images is working.

subscribe

Unfortunately, I'm on vacations next week. So, thom01, go ahead with the 6.x port! Your latest patch already looks promising -- just remove obsolete lines in your patch, f.e. those commented-out in hook_menu(), and please try to avoid unnecessary changes that do not belong in this patch, f.e. those line feeds in img_assist_filter(), img_assist_thumbs(), or img_assist_properties() of your patch.

Don't know the current status of this issue - setting to PNW for now; please always change it accordingly, and even more importantly, assign this issue to yourself if you're still working on it.

As requested assigned this to me.

I will clean up the code as soon as possible.
As there is no official version of the TinyMCE module for 6.x yet I tested the patched img_assist with the version of akie #180436: Update TinyMCE for 6.x and clean up any code that is not to the Drupal specification (#34) (or go directly to http://akie.nl/2008/03/16/tinymce-for-drupal-6/). Seems to work.

There's one issue with the redirection after uploading a new image via img_assist. I'm still working on this.

Just in case: use this in a form validate() or submit() function:
$form_state['redirect'] = 'internal/path';
@see http://drupal.org/node/144132#form-state

Thanks thom01!

So here's a new version of the patch:

- cleaned up a bit
- redirection after uploading an image is working now with a workaround

Bugs:

- header frame of the img_assist panel doesn't reload correctly

subscribing

I downloaded the current dev branch and applied the patch. The patch did not generate a .rej - hopefully it worked correctly. I then modified the .info to update the core to 6.x and dependencies to an array. img_assist now loads and I can do rudimentary work with it. I am getting an error when it attempts to load the taxonomy -

Fatal error: Call to undefined function taxonomy_get_vocabulary() in /home/dave/public_html/sites/all/modules/img_assist/img_assist.module on line 594

The site has Image Gallery taxonomies defined (about 66 total entries - 16 top level, the rest are secondary nodes). I'm getting the error on the initial thumbnail dialog and in the editing dialog.

The great news is that I'm able to generate good HTML into my blog body test with the patched version - even if I can filter yet!

I'll watch for new patches and perform regression as I see them.
-------------------------
U P D A T E
I replaced the call for taxonomy_get_vocabulary with taxonomy_vocabulary_load and the taxonomy is now loading correctly.

The layout is a bit wonky with the Barron theme - I'll dig around a bit in the CSS to see if I can get it happy.

@DaveGV: Thank you for this one.

New patch attached.

Ok, here's the next one:

Header frame reloads correctly now.

Thanks!

Time for a dev release?

Nice work! I think we're almost there. Here are a few things that I saw while having a quick look at the patch:

* Please move img_assist_menu() back up where it was – before img_assist_init() – so that it can be compared to the original.
* You should probably not comment out those two rows in img_assist_nodeapi(). The global variable defined there is used in img_assist_node_form_submit().
* The row starting with drupal_goto() in the same function is not correctly indented.
* In img_assist_thumbs(), I'd like $num_rows to be initiated before it's altered in the while loop. (Otherwhise this may cause PHP warnings.) Take a look at how they're doing it in the module upgrading guide.
* There are still a few changes (only whitespace I think) that have nothing to do with this issue. Please remove them for now.

* img_assist_menu moved

*The global variable in img_assist_nodeapi is not used at the moment. Neither are img_assist_form_alter and img_assist_node_form_submit (at least they do just nothing). I didn't get this to work. The $form_state['redirect'] got always overwritten by the redirection of the original form (image -> to the new image node), don't know why. (You can test this by putting a watchdog in form.inc) The drupal_goto is just a workaround to go back to the img_assist form.

*drupal_goto should be correctly indented now.

*img_assist_thumbs: As I see it, this would become very complicated as we have four different queries here and for all of them we'd need an extra one to count the rows just to look, if there are any. And, we have an additional query every time the page opens. In my opinion it would be better to leave it for now and have a look later if this whole thing could be done better. What do you think? At least it is working and for me it seems to be valid php. (I'm not an expert though. Maybe you have further information on this?)

*Will have a look for the rest tomorrow.

Edit:
New patch will follow tomorrow, too.

img_assist_nodeapi(): OK, so those parts still need some work.

img_assist_thumbs(): No, look at the second example in that section on the same page. It doesn't use any extra queries. The only difference between your solution and theirs is that $num_rows is defined as FALSE before the loop and then set to TRUE anytime the loop runs. ($num_rows++; will cause PHP warnings with certain configurations if $num_rows is not already defined.) You could still do it with integers if you like, just make sure the variable is properly defined ($num_rows = 0;).

Haven't looked at your most recent patch yet.

img_assist_thumbs(): I found this five minutes later after reading it again. Stupid me.

I deleted the new patch. Something went wrong. I will have a look tomorrow morning. (23:51 here now)

So here is a new patch.

* All cleaned up
* Found an error in img_assist_menu (call to img_assist_header).
* img_assist_thumbs should be clean now

Will have another look for the img_assist_nodeapi/redirect thing to get rid of the drupal_goto in img_assist_nodeapi.

I'd like to jump on this train... created a complete set of patches. The changes in detail:

• hook_menu(): Don't put title and description in t(). Callback arguments should be arrays. Removed redundant code (f.ex. 'access callback' => 'user_access' can be left out, it's the default).
• hook_nodeapi()/img_assist_node_form_submit(): No module should execute a redirect in hook_nodeapi() or a submit handler, because that will terminate the script. This means that any code in other module's nodeapi/submit handlers that would normally be run after img_assist would be skipped. Instead, I've replaced the submit button callback to point to a modified copy of node_form_submit(), which fulfills our needs without breaking foreign code. (Note: should be backported to 5.x)
• Fixed img_assist_display(). If I'm not wrong, we can now directly call image_display() (at the end), because the code is exactly the same as in image-6.x-dev.
• Fixed several l() calls.
• Several other fixes.

Still to do: img_assist button should not be displayed for invisible form elements (that is, it should be hidden as well). For example, the teaser textarea is initially hidden but the img_assist button is displayed, which leads to an ugly error message when the JavaScript tries to insert the image (insertToEditor()):

uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMNSHTMLTextAreaElement.selectionStart]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: http://localhost/sites/all/modules/img_assist/img_assist_textarea.js :: insertToEditor :: line 90" data: no]

Awesome, thanks for your efforts!

About the dirty redirection stuff: it would be nice if we could use something like $_GET['destination'] for this. Like this: img_assist/upload?destination=img_assist/properties/$nid. The problem is, obviously: we don't know what $nid is.

I can see why the drupal_goto() isn't optimal, but it would be nice to avoid the code duplication. Is there really no other way?

Anyhow, this doesn't really have anything to do with the D6 upgrade, right? I guess it could be handeled in a separate patch. Or is the current solution impossible in Drupal 6?

Another thing: there are still a few changes that are unrelated to the upgrade, like the rewritten img_assist_form_alter(). As much as they are probably very reasonable changes, it'll be easier to review and commit these patches without them.

I still haven't done an in-depth review of these patches but I'll be happy to do that once things are a little more settled. I'll probably need some help from sun and/or others to get this committed though.

So, this needs work IMO.

Great idea! Let's get rid of it by calling the original function and modifying $form_state afterwards (d'oh! so simple).

Looks good now (at least for my eyes). Good to have someone who knows what to do and where ;-) I was looking for a solution for the redirect for hours without success. Much to learn, I think.

So the only real problem seems to be the hidden form elements.

Attached a new patch:

* Deleted the @todo line at img_assist_nodeapi as the global variable is already gone

Hi

What do I do with that last patch do i switch t with the img_assist.module file?

Thanks

These are just the changes to img_assist.module. Have a look here:

http://drupal.org/patch/apply

Thanks Thom01 went throught the docs and the video was quite useful but I think it is a little over my head at the moment.

From what i can gather patches are just used for development? I'm new to Drupal how long does it usually take before and updated module is relased?

@trefor: That's hard to say. It's done when it's done. We are on a good way here, so there will be a first dev version soon, I think. If you want to test what we have now, you can download the patched module here (patch from #36, won't be updated):

http://test.ofobi.de/node/59

Feedback would be nice.

(Hope this is ok, zoo33, sun?)

Hi thom01

Yes realise things can take time but as I said I'm new to Drupal and not sure what the normal procedure on updates is.

Right now I am engaged in theming my site and getting a bit lost trying to clean up the divs and the html. If i get a chance to try the patch I will, but tbh not too confident on doing that atm. HTML and css I can do but the php side of things I'm not to hot at. Will see if I can give it a bash over the weekend.

thom01, zoo33, smk-ka - Wow - you guys are really working hard on this! I've re-patched and applied the updates. I'm going to do some nosing around to learn more about how this is working before I ask some stupid question :) I'll let you know if my system demonstrates any serious functional issues. For now - T H A N K S !

I've looked through the patches (#36 and #30) more properly now and I think they look pretty damn good. I'm attaching a new version of the main patch where I have removed a couple of minor changes that weren't related to the update.

Only one thing, I don't quite follow what's happening with the change to img_assist_display():

Fixed img_assist_display(). If I'm not wrong, we can now directly call image_display() (at the end), because the code is exactly the same as in image-6.x-dev.

It's definitely a nice cleanup, but what is it that allows us to do this now and why couldn't we do it before? (The removed stuff stems all the way back to the big rewrite for 4.7 done by benshell.)

Then there is also the issue of how to tackle the javascript errors mentioned above. So, this still needs work after all. And lots of testing. DaveGV and everyone else who's testing this – is it working?

Thanks for your great work guys!

I've been running the latest patched version, and I've been noticing the JavaScript errors almost every time I use image assist. I think it may be something to do with the fact that I've been running in 'maintenance mode'.

Sorry, forget that! I'd just been clicking on the link above the text box, rather than the one below it!

happy to help testing if you have a dev version but not confident about using patches right now.

@zoo33:

It's definitely a nice cleanup, but what is it that allows us to do this now and why couldn't we do it before? (The removed stuff stems all the way back to the big rewrite for 4.7 done by benshell.)

Oh, I wasn't aware of that fact. I just looked at image_display() in the 6.x branch and the code seemed identical, so I ripped the duplicate code out. Didn't look at 5.x code yet, but even better if it can be backported.

Well, it does seem to work with the simplified version using image_display().

I've done some testing now and I haven't found any problems apart from the js error with the hidden teaser field. I have a suggested workaround for that which is as follows:

Check that $element['#disabled'] is not true before adding the img_assist link/button to any textarea. The disadvantage would be that you'll never be able to add images to those textareas even after they've been enabled.

I've thought of another solution which changes how things work so that the img_assist link/button is added through javascript instead of being inserted in the markup. This could be done so that whenever a textarea is enabled the link/button is added dynamically. A bonus of this approach is that img_assist will not be shown to users who don't have javascript enabled.

I don't know enough javascript to do the second solution easily. Perhaps someone else knows a good way to do it? Or maybe the easy approach I mentioned first is good enough for now?

I haven't tested with TinyMCE yet. Don't know if there's even a usable version of that module for Drupal 6.

Update: I'm also getting a bunch of PHP notices. I created a separate issue for that.

Ok, a new patch from me including *everything* since a lot of files have changed:

img_assist.css and img_assist.js have been split according to their purpose:
- img_assist_popup.* are now solely responsible for everything related to the popup window.
- img_assist.css contains just the display related styles, while
- img_assist.js contains some jQuery magic to attach the buttons.

TODO: strings used in img_assist.js are not localized. Find out how Drupal.t() works and pass properly localized strings to JavaScript.

(Status update)

I think we should take the patch from #43, find a solution for the img_assist link/button problem with disabled textareas and then make a dev-version out of this. This is a nice and somehow private thread but we will have more people testing everything when there`s a dev-version they can install. It's a good idea to split the .css and .js files but I don't see why this can't wait.

@smk-ka: Does your jQuery magic solve the problem mentioned above? (I only had a quick look at the patch and don't think so.) Shouldn't we register a Drupal.behavior instead of using Drupal.jsEnabled? (Seems to be the Drupal 6.x way).

I second what thom01 says.. happy to help with testing if you have a dev version.

I haven't looked at #49 yet, but it sounds like the Right Thing To Do. It would take some work to review that patch properly though (compared to #43 + #48) so if we want to roll out something quickly we could go with the smaller patch for now and keep working on #49 separately.

(Note: If we do that, remember to state known issues: Unable to add images to teaser field, untested with TinyMCE etc.)

I'd like to hear sun's thoughts about this. I'll ping him.

Well, call me stupid, but how do i apply this patch?

@thom01: Yes, #49 solves the remaining issue. Thanks for pointing me to the "Drupal 6 way" of JavaScript behaviors, in the meantime I've rewritten the button attachment code accordingly, but would like to wait for a D6 branch instead of posting huge patches.

@zoo33: Creating a branch using the patch from #43 would really help!

subscribe

To what?

applied patch from #49 and seems to work for textareas

looking forward to the further development
thank you

Thanks for all the hard work, guys!

I have committed #30 and #43 to HEAD now. A development snapshot for 6.x should be available with the next 12 hours.

#49 looks promising, too - however, if we start to touch the JavaScript code, we want to make sure that the DRUPAL-5 branch is still maintainable (i.e. comparable) afterwards. Thus, we should work on all related issues separately, i.e.:

• JavaScript errors due to global function name clashes
• Only load img_assist.js when necessary (popup.js)
• PHP notices

Question: Can the SQL query/table changes throughout the module be backported to 5.x?

Attached is a diff between DRUPAL-6--1 and #55.
Two questions:

a) BASE_URL variable has been removed from JS, but popup JS relies on it. IIRC, D6 already provides this variable globally in Drupal.settings?
b) The img_assist_link variable default has been changed to 'icon' and looks like a bug in the form to me?

I still don't understand how to apply this patch.

@nicjasno: You do not need to apply a patch anymore. However, if that is not your point, see http://drupal.org/patch/apply

Should I continue to use this thread for issues with the dev version?

First of all thanks to all you who are working on this it is out of my league so glad you lot have some time to make the port to 6.x

Anyway I've installed the dev version but unless I am doing something daft its not working. When I go to upload a new image it opens a popup (is this really the best way to go?) I create the new image and click insert. Some text appears in my post

[img_assist|nid=48|title=Black Belt|desc=|link=none|align=left|width=330|height=253]

But when I view the post only this text appears but no image..

Also is there a way we could remove this pop up and styling option? I know you are probably trying to make it user friendly but if we place styling here surely this will be adding to the mark up?

Anyway trying to be constructive and not negative and would just like to thank you again for the work on this.

Update..

Ok been playing around with the options and i've got it to work using html imput as opposed to filter tag.

However I still think all that styling being placed inline is not the best way to go.

I think just a simple img tag would be best to allow styling to be done by the main template.

@trefor: If filter tags are still not working for you, please create a new bug report. However, please ensure that you have enabled Image assist's input filter for the input format you are using, in front of posting a new issue.

@sun: thanks i'm new to the drupal community and still trying to get my site running properly. For the moment I've stopped using image assist and am just trying to get image to work how i want it but hope to get back on it next week.