Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Patch attached :)
Comment | File | Size | Author |
---|---|---|---|
#7 | bg_image-d7_backport-7-2094331.patch | 32.05 KB | drclaw |
#6 | 2094331-d7-backport.patch | 27.01 KB | caktux |
#1 | d7-backport.patch | 28.36 KB | caktux |
Comments
Comment #1
caktux CreditAttribution: caktux commentedComment #2
drclaw CreditAttribution: drclaw commentedThanks for the patch, but it doesn't apply. I think you might have had your git refs mixed up when you did your diff?
Comment #3
drclaw CreditAttribution: drclaw commentedNope. 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?Comment #4
caktux CreditAttribution: caktux commentedI 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
butfile_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.Comment #5
drclaw CreditAttribution: drclaw commentedHi
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: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 toimagecache_create_url()
Does that all make sense?
Thanks!
drclaw
Comment #6
caktux CreditAttribution: caktux commentedHey!
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 :)
Comment #7
drclaw CreditAttribution: drclaw commentedCool, 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
Comment #8
caktux CreditAttribution: caktux commentedAHAH 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. :)
Comment #9
drclaw CreditAttribution: drclaw commentedHm, 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
Comment #10
drclaw CreditAttribution: drclaw commentedFinally committed. Sorry for the wait!
https://drupal.org/commitlog/commit/23152/2884e2a080ec156647cc6aed75f7c6...
Comment #11
drclaw CreditAttribution: drclaw commented