Taitava / silverstripe-slickcarousel

A Slick carousel wrapper for SilverStripe. See: http://kenwheeler.github.io/slick/
MIT License
1 stars 1 forks source link

[Warning] Illegal offset type #9

Closed spekulatius closed 4 years ago

spekulatius commented 5 years ago

Hello @Taitava

I've got an upgrade project using your module and found something odd after upgrading from SS3 to SS4. The following error gets thrown on pages using the carousel extension:

image

I've followed it back and found the problem in line 39 of CarouselExtension.php:

$gridfield_config->addComponent(Injector::inst()->create($sortable_rows_class,['Sort']));

The 'sort' is an array while a string is expected to match the key in the error. Not entirely sure why it is an array. Should I submit a PR for this?

Peter

Cheers, Peter

Taitava commented 5 years ago

Hi Peter!I'm currently on a holiday trip which will last for a few weeks. After that I will have a better chance to examine this issue (now I'm writing this message by using my cell phone). Anyway, a pull request sounds good and I will check it after my trip. Thank you for your effort! :)While your pull request will be waiting to be merged, you can use your own forked repository in composer.json to be able to apply the fix in your project. Please let me know if you need further instructions on how to do this.11.7.2019 10.28 Peter Thaleikis notifications@github.com kirjoitti:Hello @Taitava I've got an upgrade project using your module and found something odd after upgrading from SS3 to SS4. The following error gets thrown on pages using the carousel extension:

I've followed it back and found the problem in line 39 of CarouselExtension.php: $gridfield_config->addComponent(Injector::inst()->create($sortable_rows_class,['Sort']));

The 'sort' is an array while a string is expected to match the key in the error. Not entirely sure why it is an array. Should I submit a PR for this? Peter Cheers, Peter

—You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or mute the thread.

spekulatius commented 5 years ago

Hello Jarkko,

no problem at all. Enjoy your holiday. I'll prepare a PR over the next days and send it to you - no rush on checking it out. I'll add my fork as a dependency for now :)

Peter

On Fri, 12 Jul 2019 at 17:05, Jarkko Linnanvirta notifications@github.com wrote:

Hi Peter!I'm currently on a holiday trip which will last for a few weeks. After that I will have a better chance to examine this issue (now I'm writing this message by using my cell phone). Anyway, a pull request sounds good and I will check it after my trip. Thank you for your effort! :)While your pull request will be waiting to be merged, you can use your own forked repository in composer.json to be able to apply the fix in your project. Please let me know if you need further instructions on how to do this.11.7.2019 10.28 Peter Thaleikis notifications@github.com kirjoitti:Hello @Taitava I've got an upgrade project using your module and found something odd after upgrading from SS3 to SS4. The following error gets thrown on pages using the carousel extension:

I've followed it back and found the problem in line 39 of CarouselExtension.php: $gridfield_config->addComponent(Injector::inst()->create($sortable_rows_class,['Sort']));

The 'sort' is an array while a string is expected to match the key in the error. Not entirely sure why it is an array. Should I submit a PR for this? Peter Cheers, Peter

—You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or mute the thread.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Taitava/silverstripe-slickcarousel/issues/9?email_source=notifications&email_token=ACAK7MZGRXNC7WFS4ZEK7U3P7BJQLA5CNFSM4IAYGNVKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZZKRPA#issuecomment-510830780, or mute the thread https://github.com/notifications/unsubscribe-auth/ACAK7M23KWPQSEFJV5V5Q2LP7BJQLANCNFSM4IAYGNVA .

spekulatius commented 5 years ago

Hello Jarkko,

I've opened the PR for you. No need to check it now, it can wait until you are back.

Peter

On Sat, 13 Jul 2019 at 11:52, Peter Thaleikis peter.thaleikis@gmx.de wrote:

Hello Jarkko,

no problem at all. Enjoy your holiday. I'll prepare a PR over the next days and send it to you - no rush on checking it out. I'll add my fork as a dependency for now :)

Peter

On Fri, 12 Jul 2019 at 17:05, Jarkko Linnanvirta notifications@github.com wrote:

Hi Peter!I'm currently on a holiday trip which will last for a few weeks. After that I will have a better chance to examine this issue (now I'm writing this message by using my cell phone). Anyway, a pull request sounds good and I will check it after my trip. Thank you for your effort! :)While your pull request will be waiting to be merged, you can use your own forked repository in composer.json to be able to apply the fix in your project. Please let me know if you need further instructions on how to do this.11.7.2019 10.28 Peter Thaleikis notifications@github.com kirjoitti:Hello @Taitava I've got an upgrade project using your module and found something odd after upgrading from SS3 to SS4. The following error gets thrown on pages using the carousel extension:

I've followed it back and found the problem in line 39 of CarouselExtension.php: $gridfield_config->addComponent(Injector::inst()->create($sortable_rows_class,['Sort']));

The 'sort' is an array while a string is expected to match the key in the error. Not entirely sure why it is an array. Should I submit a PR for this? Peter Cheers, Peter

—You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or mute the thread.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Taitava/silverstripe-slickcarousel/issues/9?email_source=notifications&email_token=ACAK7MZGRXNC7WFS4ZEK7U3P7BJQLA5CNFSM4IAYGNVKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZZKRPA#issuecomment-510830780, or mute the thread https://github.com/notifications/unsubscribe-auth/ACAK7M23KWPQSEFJV5V5Q2LP7BJQLANCNFSM4IAYGNVA .

Taitava commented 4 years ago

Hi Peter,

sorry that this took so long, but I have now checked this.

The 'sort' is an array while a string is expected to match the key in the error. Not entirely sure why it is an array.

It seems that this was a mistake that I made when upgrading this module to SS 4. While I did the upgrade, I changed the way how an instance of UndefinedOffset\SortableGridField\Forms\GridFieldSortableRows class is created to use SilverStripe's Injector class to create the instance (just to make instance creation to comply with the standard SilverStripe way) (commit 7ea55b3a4637b28ca92e428b2aed0733b329a320). While doing this change, I somehow managed to screw up the 'Sort' parameter. Perhaps I thought that the create() method of the Injector class would want to take pass through parameters as an array, but for some reason I didn't check it.

So your solution seems to be correct. Thank you for your effort! :)

Taitava commented 4 years ago

Fixed in #10.