Patch attached :)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

caktux’s picture

FileSize
28.36 KB
drclaw’s picture

Status: Active » Needs work

Thanks for the patch, but it doesn't apply. I think you might have had your git refs mixed up when you did your diff?

drclaw’s picture

Status: Needs work » Postponed (maintainer needs more info)

Nope. Never mind. That was my bad. I was applying it to the d7 branch. Derp.

Although I managed to get your patch applied to the 6.x branch... I don't understand what the patch is supposed to do. It totally breaks functionality. There are function calls in there to functions that weren't introduced until D7 (e.g. path_is_admin(), ile_create_url()). Additionally the AHAH javascript was removed and replaced with the "AJAX" functionality which was also not introduced until D7. It basically looks like you just did a diff between the drupal 7 and drupal 6 versions of bg_image.module ... Is that correct?

caktux’s picture

I didn't just do a diff. AHAH has been replaced with AJAX if you look at the patch. You're right about path_is_admin but file_create_url is in D6 and its differences have been accounted for.

Your D6 version was missing the imagecache support, background-size option, media query support and the CSS is broken with !important hard-coded in it.

Just get path_is_admin() back in with a prefixed name, some people might already have copied that function directly from D7. I'll take five minutes this weekend to do it if you don't have time. It should be good to go after that, I've been using that exact version for weeks and since I never checked the admin thing I never noticed.

drclaw’s picture

Hi

Ah, I see. Sorry, I didn't mean to jump to any conclusions there. At first glance it just looked like there were a lot of D7isms left behind so I thought maybe you'd diff'd the wrong refs or something in git. My mistake!

Okay so, the only major note I have is regarding the ajax functionality. Are you using the AJAX Module by any chance? The #ajax form property was not introduced until drupal 7. In drupal 6 I believe it's the ajax module that brings that functionality to the table. I would prefer to go back to the core #ahah functionality rather than introduce a module dependency...

Regarding the path_is_admin function, it was actually there before, but the patch removes it here:

 /**
  * Returns an options list of css repeat options
@@ -375,14 +573,34 @@ function bg_image_css_repeat_options() {
   );
 }
 
+/***********************************************************************
+ * HELPER FUNCTIONS - Private
+ ***********************************************************************/
+
 /**
- * Returns true if the current page is an 'admin' page
+ * Converts a path to a uri relative to the public files directory
+ *
+ * @param $path
+ *    The path to convert. Can be an absolute or relative path. If the path
+ *    contains the public files directory in it, it will be stripped before
+ *    building the uri, since we're building a public:// uri
+ *
+ * @return
+ *    The path to the file as a uri.
  */
-function path_is_admin() {
-  if (arg(0) == 'admin') {
-    return TRUE;

Lastly, I don't think we need the function _bg_image_path_to_uri at all since D6 didn't use stream wrappers. We should just be able to use the $image_path directly in in the call to imagecache_create_url()

Does that all make sense?

Thanks!
drclaw

caktux’s picture

FileSize
27.01 KB

Hey!

The ajax part is actually useless and should be removed, it's not doing anything and it works a lot better without. Node types and fields are simply loaded with the form in a two step process. Not ideal but no dependencies or broken ajax.

path_is_admin should have stayed there but we should just prefix it's name. I'm all for simplifying the path handling if it works, I'm quite certain you're right.

Anyway, here it is, while looking at the code and writing, might as well do it :)

drclaw’s picture

Issue summary: View changes
Status: Postponed (maintainer needs more info) » Needs review
FileSize
32.05 KB

Cool, thanks for all your work on this so far!

However, I think we're not understanding each other on the AHAH/AJAX thing. Without your patch, the ajax works fine on the settings page. You select a node type, and the field list refreshes automatically. This uses AHAH which is built into core in D6. No dependencies.

It's only with your patch where the AHAH functionality is removed and D7 AJAX functionality is inserted that it breaks. I agree that we should remove the D7 AJAX stuff from the patch, but we should also retain the AHAH functionality that was there and working all along.

Additionally, I found a couple more D7-isms in your patch that won't work. They were both database query related. I've fixed those and updated the patch. It would be great if you could take a look when you get a chance.

Thanks!
drclaw

caktux’s picture

AHAH still breaks with i18n, I guess that must have been my initial problem which pushed me to remove it. I do think you're right about all this though, your patch looks great and I don't see why this would fail, must be a core or i18n issue or both. All my functional tests passed besides that. I tried to find a quick fix but I'm seriously tired of trying to fix those i18n issues with every module, especially when it doesn't seem to be the module's fault like this here.

Thanks a lot for the follow-up, I'm very happy to see a maintainer still caring for D6. It's still a great platform, between D8 being so unstable and D7 being so slow, I almost get to love D6's limitations. :)

drclaw’s picture

Hm, that's strange about the i18n stuff... I'm going to commit this patch for now, but hopefully I'll have a chance to explore it a bit further in the near future. If you figure anything out, maybe open a new issue?

Thanks again!
drclaw

drclaw’s picture

drclaw’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.