Closed (fixed)
Project:
Picture
Version:
7.x-2.x-dev
Component:
Code
Priority:
Critical
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
9 Sep 2013 at 15:54 UTC
Updated:
20 Jun 2014 at 17:40 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
jwilde commentedNice catch. Changing it to, template_process_flexslider_list fixed it for me. JIm
Comment #2
abdullahgz commentedIs there a patch for this issue
Comment #3
drupalgideonNot sure if the same thing, but I get the following error after updating to alpha 3
Notice: Undefined index: uri in theme_flexslider_picture_list() (line 56 of /sites/all/modules/contrib/picture/flexslider_picture/theme/flexslider_picture.theme.inc).The $vars['items'] array passed to the theme function in alpha 1 has a 'thumb' element in the array (full path to thumbnail image), but has 'caption' in alpha 3 (in my case this is NULL).
Comment #4
jantoine commentedThe attached patch updates the template_preprocess_flexslider_picture_list() function to be template_process_flexslider_picture_list() (a process function instead of a preprocess function) to match the flexslider module. It also fixes the calls to the updated functions in the flexslider module.
Comment #5
chris burge commentedThank you for writing this patch. My slider works again; however, I now get the following notices:
I don't know if what I am describing is a symptom of this particular issue or another issue that hasn't been reported yet.
Set up Info
Using Views to create slider:
Codebase
Comment #6
dieuweThanks for the patch, it works for me. Don't see any errors like #5 right now, but I will post back here if I do.
Comment #7
dieuweI enabled the flexslider thumbnail paging controls and now I get the same messages as #5.
Comment #8
dieuweIt seems to me that flexslider has changed the way it handles thumbnails. I wouldn't know where to start looking though.
Comment #9
gilsbert commentedThe patch #4 worked for me too.
May it be commited?
Comment #10
attiks commentedComment #11
darren ohFixed in commit 20473c8.
Comment #12
das-peter commentedJust wondering, has anyone got responsive images with this patch? All I seem to get are the normal flexslider images - not the ones from picture.
To make that happen I had to patch the module as seen in the attached patch.
For me it looks like not only the method name changed but also the return parameter - which needs different handling now.
Attention: This changes make (including the already applied patch) make picture flexslider incompatible with flexslider versions prior alpha 3. I'm fine if that's intended, but I think it should be documented.
Comment #13
alexander.sibert commentedI have the same issue:
Setup:
I use Panels 3, Views with Flexslider, Picture, Breakpoints. Here you see http://lafox.de - the Flexslider works but i receive errors on top.
Update
I updated now Picture/Flexslider and the Flexslider Library to current Dev releases but the errors are not away.
Comment #14
dieuwedas-peter: No, responsive images were not fixed in the patch, as you will see from comment #5 in particular.
I'll try the patch in #12 sometime next week, I'm just too busy right now. I too would be fine with breaking everything prior to flexslider alpha 3 as this is this module obviously needs to keep up.
Comment #15
dieuweCurrent dev code with flexslider thumbnail paging enabled:
Patched from comment #12:
Comment #16
das-peter commentedUpdated the patch to fix the integration into flexslider >= alpha3.
Changes:
In the latest flexslider version the image style is set in the field formatter and not in the optionset itself. We should do the same thing and move the selection of the mapping to the field formatter settings in a next step.
theme_flexslider_list_item- set the proper settings array.flexslider_picture_field_formatter_viewto avoid processing not available data.It looks like quite some parts of the flexslider code was changed. And as far as I can see it looks much cleaner now.
The option set now only defines how the flexslider behaves and not which image style is used. The image style is defined in the field formatter settings now.
This is promising because I think that way we can provide our own field formatter with the settings to define the flexslider optionset and the picture group to use. This would loose the coupling and make it easier to keep up with flexslider overall.
Comment #17
das-peter commentedAttention: This patch will break your existing configuration!
The attached patch contains the big change I proposed in #16:
The configuration of the picture group / colorbox stuff doesn't reside anymore in the flexslider optionset but in the field settings of our own field formatter.
Instead hijacking theme functions we now have our own field formatter and use it (
flexslider_picture_field_formatter_view()) to prepare the items to pass them toflexslidertheming.Advances of that are:
Attention: This patch will break your existing configuration!
Comment #18
rcodinaThe patch on #17 works for me using this versions:
-Drupal 7.24
-Breakpoints 7.x-1.1
-Picture 7.x-1.2 with #17 patch
-Flexslider 7.x-2.0-alpha3
-jQuery FlexSlider v2.2.0
Also, in manage display I had to select "Picture" formatter instead of "Flexslider Picture" formater to get responsive Flexslider to work as expected.
The "Flexslider Picture" formatter does not work: additional blank slides are added and loaded slides turn to blank soon after loading.
Thank you das-peter!!!
Comment #19
robcarrPatch at #17 applied cleanly for me, and has removed numerous errors (also including #2072515: Illegal string offset 'thumb' in theme_flexslider_picture_list()).
This is the first project I've used Picture and Flexslider, and the settings now seem a little easier to use from an 'incomers' perspective. This patch would get my support, but it would be good to get a slightly wider review before setting to RTBC, as I'm not sure of any wider implications to such a substantial change to the module code.
Comment #20
FranCarstens commentedI have the same issue as "tmtheowner". Thumbnail images do not show up at all with the patch in #17 using "Flexslider Picture" formatter. My setup is exactly the same as post #18
URL display as: sites/default/files/styles//public/...
It looks like the thumbnail style isn't called.
== EDIT
Not sure if this is new, but it looks like Flexslider does not use a thumbnail image style anymore. It merely resizes the slides, thereby loading only 1 set of images. Pros and cons I guess; but this makes a module like flexslider_picture even more valuable.
Comment #21
hydra commentedIn a simple flexslider configuration without thubnail configuration and picturefill method the patch in #17 works for me.
Comment #22
Ravenight commentedI fixed this in a very hacky way but it works for now till a better solution comes along.
Using:
-Flexslider - 7.x-2.x-dev (updated as of this date)
-Picture 7.x-1.2 with #17 patch
-jQuery FlexSlider v2.2.0
In flexslider_picture.module I added the following on line 216:
Its hacky as all get-out but works and let me use picture, colorbox, and flexslider with thumbnails and will allow a thumbnail to be created if the image style does not already exist.
Comment #23
c-c-m commentedI applied the patch in #17 and it worked fine except for the fact that the slider started to display a caption (I had this option disabled) and I didn't find any place in admin UI to change that. If I am not wrong, I think this option used to be available on UI.
Comment #24
indigoxela commentedHi,
patch #17 partly solved my picture/flexslider problems.
My setup:
Before patching, error messages appeared (Undefined index: image_style ...) after patching these messages are gone.
After patching - unlike mentioned in #18 - "Flexslider Picture" formatter worked for me as expected. The correct image sizes get loaded.
What does not work: responsive thumbnails
With a normal (not responsive) Flexslider, thumbnails appear, but are only resized via css, not via image style.
With Flexslider Picture, paths to thumbnail image styles are empty like mentioned in #20, so images can't get loaded.
Should we add a "Thumbnail" setting to Flexslider Picture formatter settings?
Or should we grab it from flexslider's option set? The setting is still there, but is not in use anymore (or temporarily not in use?).
EDIT: as a reference - thumbnail discussion on Flex Slider module issue list: https://drupal.org/node/2123947 (Thumbnail image style is not being applied)
Comment #25
indigoxela commented@ c-c-m (comment #23)
I can find the caption setting on the image field's display setting (/admin/structure/types/manage/CONTENTTYPE/display),
right beneath "Picture group".
Comment #26
indigoxela commentedHi,
to come back to the thumbnail problem with patch #17 applied:
Not the thumbnail image_style itself is the problem, but the fallback_image_style.
Fallback images are broken.
Flexslider module grabs the image src via preg_match(). That would actually work even for flexslider_picture thumbnail items (although very un-responsive!), but unfortunately the variable fallback_image_style seems empty at that point.
Fallback images work in picture but not in flexslider_picture.
EDIT: oops, that problem is actually easy to fix...
$item['slide'] array (flexslider_picture.module, around line 193 if patched) is missing the #style_name setting:
...
'#style_name' => $fallback_image_style,
...
Comment #27
indigoxela commentedSo, to make testing easier, I created a patch with the changes of patch #17 plus the little modification mentioned in #26 to make thumbnails in flexslider_picture work again.
Still thumbnails are created in the new Flexslider way (simply by grabbing the normal slider image, the fallback image with picture in our case, and resizing it via css). Keep that in mind, if you set your fallback image to original image size.
Comment #28
Kojo Unsui commentedI had following problems :
With setup:
Applying patch #17 solved 1 : resizing browser now loads corresponding image style, but still no thumbnails
Applying #26 solved 2 : thumbnails appeared (make sure to select a thumbnail style, because the same style is applied whatever windows size...)
So great patch #17, thanks Das-Peter (and Indigoxela for code snippet) ! I think this could be committed to dev ASAP ?
Last thing missing is caption. I have both alt and title enabled for my images, but the formatter returns a disabled checkbox anyway (in panels). In structure/types/manage/.../display the checkbox is not disabled, but it is in Panels... Any idea to fix that ?
And what about optional image, when field is empty and returning default image ? At the moment it returns Notice : Undefined index: width in flexslider_picture_field_formatter_view() (line187 in ... sites\all\modules\picture\flexslider_picture\flexslider_picture.module).
EDIT: patch #27 is certainly ok, but at the moment I tried it I misconfigured sthing, so I reverted it and applied separately #17 and #26 ...
Comment #29
indigoxela commentedHi,
yes, there needs some more work to be done.
@Kojo Unsui: I'm afraid, I can't help with captions and panels. The problem only occurs with panels? I can't reproduce it outside panels.
@das-peter: "Notice : Undefined index: width..." when using a default image in field and there is no user-contributed image.
That could be fixed with, for example:
By checking if it is a default image, which has its dimensions set differently (row 184 in flexslider_picture.module)
Would you mind to rework your patch?
Actually I don't understand, why $item['slide'][xxx] has to be set anyway, because right after these assignments the array for theme_image_style is created from scratch and $item['slide'][xxx] isn't used anywhere.
Am I missing something?
A quick test showed, that it all works fine without extra assigning of $item['slide'][xxx].
The only thing we need is to set $item['item']['width'] (and height), for default images.
Comment #30
Kojo Unsui commentedThanks for your code about default image, I'm gonna try it.
The caption problem only occurs with panels. With same content type and same field:
May I open another issue for that specific point ?
Comment #31
Kojo Unsui commentedRight Indigoxela #29 corrects the notices for default image. I only changed
$item['item']['image_dimensions']by$item['item']['is_default'].So this goes around line 184, after
// Setup the variables for calling theme_image_styleComment #32
indigoxela commentedHi Kojo Unsui,
just curious, why did you use $item['item']['is_default']['width']?
It is NULL actually, while $item['item']['image_dimensions']['width'] holds the width of the original image (same for height).
At least this is the case in my test drupal.
Comment #33
Kojo Unsui commented@indigoxela, I used 'is_default' because in this case :
- the field image is optional
- you loaded a default image
- but no other image yet...
$item['item']['is_default']['width']is fine, while'image_dimensions'returns undefined.Comment #34
indigoxela commentedHi Kojo Unsui,
weird.
Still different here, no matter how often I flush cache.
What we are testing: the image field is optional, has a default image and user didn't add an image, so the default image is shown.
I hope at least that is clear.
We are fiddling in function flexslider_picture_field_formatter_view.
Here I have:
var_dump($item['item']['image_dimensions']);
array(2) {
["width"]=>
string(3) "960"
["height"]=>
string(3) "960"
}
var_dump($item['item']['is_default']);
bool(true)
So here a
var_dump($item['item']['is_default']['width']);
is NULL
That doesn't show any error, but image attributes (width/height) for default image are missing.
By the way: $item['item']['image_dimensions'] here always has values, even for images uploaded by users.
It is the array with width and height of the original image (not with any image style applied).
There could be some differences in our testing setups, let's see:
Drupal: 7.26
Picture: 7.x-1.2 (flexslider_picture submodule patched)
Field:
Widget: Multiupload
Required: no, optional field
Default image: yes, is set
Values: unlimited
Filesystem: public, no special subdir
Max dimensions: no limit
Any clue?
EDIT: wait a minute... you are testing it in a panel?
Possibly the original image dimensions get lost within panels.
Comment #35
Kojo Unsui commentedWell, I tested both with and without Panels. Yes the test conditions are the same :
image field optional, has a default image and user didn't add an image, so the default image is shown.
Values: unlimited / Filesystem: public, no special subdir / Max dimensions: no limit
Setup slightly different, check #28
We have the same here :
-
var_dump($item['item']['is_default'])returns bool(true)-
var_dump($item['item']['is_default']['width'])is NULL- It doesn't show any error
But
var_dump($item['item']['image_dimensions'])is NULLThis is why I altered your code.
Comment #36
prineshazar commented#17 worked for me. Thanks das-peter!
As others have pointed out.
1. Apply the patch
2. Run update.php / drush updb
3. Go to "Manage display" sections and change formatter from flexslider to "Flexslider picture".
Comment #37
indigoxela commentedHi Kojo Unsui,
ahhh, sloppy reading... you already determined that you use Core D7.24.
Another try in flexslider_picture_field_formatter_view:
This works fine here and doesn't throw any error, even if $item['item']['image_dimensions'] is not set.
This way it is a bit easier, as $item['item']['width'] and height is not needed at all (which is not set for default images).
Comment #38
Kojo Unsui commentedI'm not sure which solution is the simplest one, but #37 also is working in my setup . It's up to you.
In the meanwhile, I solved #30 Panels caption checkbox disabled (not related to Picture in fact) altering the Formatter Styles form.
Comment #39
indigoxela commentedHi,
unfortunately I found another problem: thumbnails are also broken after patch (#17, #27) when using colorbox in field display.
Probably it's easy to fix by (again) fiddling in flexslider_picture_field_formatter_view:
We should put things together for a new patch (fix for notice : undefined index, fix for colorbox thumbnails...)
Hope to find the time the next few days, but I would be glad if someone else could help.
Anyone willing to test is also very welcome.
Comment #40
indigoxela commentedHi,
here is a combination of all changes, beginning with the major change introduced in #17 by das-peter.
Also included:
#26: added "#style_name" to bring back thumbnails
#37: prevent php notice problems when using default image
#39: make thumbnails work with colorbox
#17 is a bigger change. If you haven't applied any of the patches yet you have to:
Comment #41
indigoxela commentedOK, and now the patch... ;)
Forgot to attach it in #40.
Comment #42
Ravenight commented#41 worked for me.
Comment #43
rodpal commented#41 worked great! Thanks
Comment #45
attiks commentedComment #47
Ravenight commentedI just upgraded from Picture 1.2 to 1.3 today and received errors "flexslider_picture_optionset" was missing. I dug a bit deeper and found that the #41 patch was not applied on the latest version of flexslider_picture.
It appears this was done:
and this was done:
but the part of #41 that involves the flexslider_picture.install was not applied nor some of the other major things in the patch. Just an FYI that you will have to re-apply this patch to the 1.3 version of pictures' flexslider_picture if you find you are getting errors after the update. I did this and now my site is working again.
If #41 should have been multiple patches, that should be resolved in separate issues.
Comment #48
attiks commentedCan you create a new patch?
Comment #49
robcarrHad been using the patch at #17 and working fine.
Upgraded module to 1.3 and was riddled with errors. Thought I'd upgrade to 7.x-2.1 and greeted with WSOD (Call to a member function getMappings() on a non-object in .../picture/picture.module on line 1062). Although this is a separate issue related to mappings #2274661: Fatal error: Argument 1 passed to picture_get_mapping_breakpoints() must be an instance of PictureMapping I've got no way of rolling/testing out a new patch, so it's worth flagging up here in case anyone else is trying to looking into this Flexslider issue and hits the same brick wall.
Comment #50
robcarrA new patch is required for review
Comment #51
robcarrI've attached a patch rolled against 7.x-2.x... however, I can't test it due to the problem at #2274661: Fatal error: Argument 1 passed to picture_get_mapping_breakpoints() must be an instance of PictureMapping. Let me know if it works!
Comment #52
indigoxela commentedHi,
I knew, #17 is sort of dangerous. It dropped database table flexslider_picture_optionset.
How can we proceed safely? Especially all users who already applied #17 might be forced to completely uninstall and re-install this submodule. Even then eventually direct interaction with the database might be required.
Any ideas? das-peter?
For those who struggle with missing db table errors (PDOException: SQLSTATE[42S02]: Base table or view not found), here is the table structure to recreate it in MySQL (empty!) as sql command:
Do not apply this if you don't know what the above code means!
Comment #53
jelle_sFlexslider & flexslider views is fixed for 7.x-1.x, now starting on 7.x-2.x (since #2274661: Fatal error: Argument 1 passed to picture_get_mapping_breakpoints() must be an instance of PictureMapping is now fixed as well)
Comment #54
jelle_sComment #55
jelle_sComment #56
das-peter commentedAwesome, thank you all very much for continuing working on this!! :)
Comment #57
indigoxela commentedHi Jelle_S,
I reverted all changes on my testing instance made by patches created here.
I can confirm that all error messages are gone after updating picture to 7.x-2.1+8-dev.
Unfortunately this doesn't mean, that flexslider_picture is fully functional.
I really don't want to discourage you but...
I checked
and all looked OK.
Markup looks good, "picture" and "source" tags are set correctly.
(Although I would like to mention that fallback images won't ever work as they don't have a src attribute. But that's a picture issue and is not introduced with this submodule.)
Now the problem:
The image_style actually in use for flexslider displays is the FIRST in image styles list of the picture mappings
(/admin/config/media/picture/list/MAPPING/edit).
This style is also the first "srcset" attribute.
When resizing the screen, no adaption takes place, still the first image of the mapping is used.
All the other picture mappings work just fine, only the flexslider picture responsive images are not responsive.
Am I missing something?
Comment #58
jelle_s@indigoxela: Have you cleared your caches? (Drupal & browser cache). Everything works fine on my end.
Comment #59
indigoxela commentedHi Jelle_S,
thank you for your quick response.
Yes, I flushed (and flushed and flushed...) Drupal cache and disabled browser cache.
Still the same problem in Firefox and Chromium.
No javascript errors or php errors at all, only the wrong image size.
Which flexslider library version are you using?
I am using the version suggested here: https://drupal.org/node/1420924#comment-7983425
Comment #60
jelle_sI'm using version 2.2.2 (https://github.com/woothemes/FlexSlider/releases)
Comment #61
indigoxela commentedHi Jelle_S ,
first I have to correct my bug report (comment #57). After the picture update even "normal" picture mappings only use the first "srcset".
So this is likely not a flexslider_picture problem but a picture issue.
Again I reverted everything and started from scratch.
And I switched to the exact same flexslider library you use.
Same result - wrong image size.
And - yes - flushflushflush.
As the html source code looks ok and there are no php errors, it is likely a javascript problem (still no errors though). Picturefill sets the image src attribute, right?
Is there another picturefill version I could try?
Should I try a different picture (dev-)version?
What I've used so far is the official 7.x-2.x-dev version available on the project page.
Maybe I'd better start with 7.x-1.x-dev.
But let us wait a bit, what results other testers get.
Comment #62
attiks commentedKeep in mind that the dev versions are build every 12 hours, so you have to wait a bit or use git clone
Comment #63
jelle_s@indigoxela: can you also print the output of the picture HTML here?
Keep in mind that the least specific breakpoint should appear as the last source element. So in stead of:
it should be:
Because picture selects the first matching source element.
You can change the order of the breakpoints at admin/config/media/breakpoints/groups/breakpointgroup
Where breakpointgroup is the machine name of the breakpoint group you selected for your picture mapping.
Comment #64
indigoxela commented@Jelle_S
excellent!
I had to change the breakpoint order to make it work.
In other words, the largest image should be the first. When did that change?
It used to be the other way round and still is in the documentation:
https://drupal.org/node/1904690
Now I can confirm that the update from picture 7.x-1.2 to 7.x-2.1+8-dev works.
One caveat: fallback images don't work yet as the img tag has no "src" attribute without javascript.
Keep in mind that flexslider thumbnails won't work without functional fallback images.
Comment #65
attiks commentedFallback without js does indeed not work, but if we add a src on the img tag, it will download 2 images.
What is the flexslider problem with thumbnails?
Comment #66
indigoxela commented@attics:
Not if you use a "noscript" tag around the fallback image tag. OK, that would require some more html code, but I suppose, a functional fallback solution is desirable.
Flexslider library and as a result flexslider module completely changed thumbnail handling.
The problem was discussed here and is also discussed in flexslider issues (e.g. https://drupal.org/node/2123947).
In short: thumbnail image style settings are skipped and instead of that the slider images will be resized via css.
Flexslider module uses preg_match() to grab the first src attribute found for thumbnail html code creation.
Comment #67
attiks commentedCan you create a new issue for "Not if you use a "noscript" tag around the fallback image tag"
This sounds like a strange approach, since I don't use the flexslider module no idea on how to solve this, best to handle in a new issue as well
PS: Thanks all for all the testing
Comment #68
attiks commentedComment #69
jelle_sThis is already fixed, see http://cgit.drupalcode.org/picture/tree/flexslider_picture/theme/flexsli...
Comment #70
das-peter commentedSomething is strange here. It looks like essential parts of the patch weren't committed.
A good example is
flexslider_picture_field_formatter_infoWhat's in the repo:
What the patch contains:
Was this done on purpose? There was no objection related to those parts, right?
As far as I can tell the re-roll of arrrgh in #51 should have been committed as is.
Comment #71
jelle_sI'll have a look
Comment #72
jelle_sOk, first of all, the patch can not be committed as-is.
As said in #17, it breaks existing configuration without an upgrade path:
The only difference in features I can see, is the colorbox support. So for now, I'm inclined to add the colorbox support without rewriting the entire module. The original issue was to fix the module so it would work with flexslider >= alpha3. A major rewrite can be considered in a separate issue, if there is a decent upgrade path.
Comment #73
das-peter commentedThanks for looking into that - totally missed #17 then, my bad. :|
Comment #74
jelle_sColorbox is now supported (it was before as well, but with fixing this issue something got messed up).
Comment #75
das-peter commentedCan't figure out a proper upgrade path right now, so the attached patch just introduces the new approach besides the "legacy" one.
Comment #76
jelle_s@das-peter: Thank you for working on this, but could you move it to a separate issue? The original issue is now fixed. It's more clear if we could keep things separated.
Comment #77
das-peter commentedOkay then, here we go: #2281503: Change approach for flexslider integration