Problem/Motivation

After the release of views 3.16 this module generates a lot of warnings, due to the $join->extra being set with a string instead of an array.

Proposed resolution

Simply refactor the code generating the warning by building the extra clause as an array.

Remaining tasks

Review (this being my first patch ever :-))

User interface changes

The user interface is not affected.

API changes

No API changes.

Data model changes

No data model changes.

CommentFileSizeAuthor
#6 select_translation-warnings_with_views-2876095-6-D7.patch500 bytesao2
#2 select_translation-warnings_with_views-2876095-0-D7.patch726 bytesnironan

Comments

nironan created an issue. See original summary.

nironan’s picture

nironan commented
StatusFileSize
new select_translation-warnings_with_views-2876095-0-D7.patch726 bytes
nironan’s picture

nironan commented
Status: Active » Needs review
ao2’s picture

ao2 commented

nironan, the patch seems OK, and after a quick research it seems that it's also compatible with Views 2.x.

I cannot test it because I don't use Drupal 7, so it'd be nice to have a RTBC by someone else, but I don't consider this a blocker.

BTW, can you paste the exact warnings that you get without the patch? And what change in Views 7.x-3.16 triggers it?
It's good to have these details in the long commit message and/or in the release notes.

Thanks,
Antonio

ao2’s picture

ao2 commented

Ping.

Can you paste the exact warnings that you get without the patch? And what change in Views 7.x-3.16 triggers it?
It's good to have these details in the long commit message and/or in the release notes.

ao2’s picture

ao2 commented
StatusFileSize
new select_translation-warnings_with_views-2876095-6-D7.patch500 bytes

The warning seems to occur in views after commit 36eef6e which makes the code rely more strongly on the assumption that the $extra parameter is an array, as actually documented in the API.

The previous code was more forgiving that it needed to be.

And in the related issue I see that the error message is

Warning: Invalid argument supplied for foreach() in views_join->build_join() (line 1548 of ./sites/all/modules/views/includes/handlers.inc).

Can you confirm the analysis?

The minimal fix in select_translation would be something like in the attached patch.

Can you please try it in place of your previous one?

Thanks,
Antonio

ao2’s picture

ao2 commented

Ping.

@nironan were you able to test the minimal patch from #6?

Thanks,
Antonio

kmonty’s picture

kmonty
He/Him
Location Berlin, DE + SF, CA
commented
Status: Needs review » Reviewed & tested by the community

I can confirm this was a bug caused by the change made to resolve #1930288: views_join: wrong handling of $extra parameter. Not only was it throwing PHP notices as the OP mentioned, this bug was causing significant performance impacts on a major client site. Patch number #6 resolved both the notices and negative performance impacts.

I will mark as RTBC, but understand if you want additional eyes on this before committing. Thank you ao2 for the quick fix!

ao2’s picture

ao2 commented

OK @kmonty thanks for the test.

I am going to apply the patch in #6 then and release a new stable 7.x version on Monday. if @nironan too finds some time to test it before then I'd appreciate that, but I don't consider this a blocker.

Thanks,
Antonio

nironan’s picture

nironan commented

@ao2, sorry for the late reply. I just managed to test your patch and it works! Thank you

  • ao2 committed 6f453a7 on 7.x-1.x 
    Issue #2876095 by nironan, ao2: fix warnings appearing with views >= 3....
ao2’s picture

ao2 commented
Status: Reviewed & tested by the community » Fixed

OK, fix pushed.

I'll release a new stable version today.

Status: Fixed » Closed (fixed)

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