search

pods-framework / pods

The Pods Framework is a Content Development Framework for WordPress - It lets you create and extend content types that can be used for any project. Add fields of various types we've built in, or add your own with custom inputs, you have total control.
https://pods.io/
GNU General Public License v2.0
1.07k stars 264 forks source link

OrderBy Failing on PHP7 #3294

Closed PodsBot closed 8 years ago

PodsBot commented 8 years ago

OrderBy Failing on PHP7 submitted via Slack by jimtrue

tomherold commented 8 years ago

The following will draw an error with php7 but works fine with php 5.5 and php 5.6. This example pod is based on a custom post type with meta storage. I have a field 'order_alpha' plain text.

    $forces = pods('forces');
    $params = array(
        'orderby' => 'order_alpha',
        'limit' => 10,
    );
    $forces->find($params);

Error message:

Database Error; SQL: SELECT DISTINCT `t`.* FROM `wp_posts` AS `t` LEFT JOIN `wp_postmeta` AS `order_alpha` ON `order_alpha`.`meta_key` = 'order_alpha' AND `order_alpha`.`post_id` = `t`.`ID` WHERE ( ( `t`.`post_status` IN ( "publish" ) ) AND ( `t`.`post_type` = "forces" ) ) ORDER BY ASC, `t`.`menu_order`, `t`.`post_title`, `t`.`post_date` LIMIT 0, 10; Response: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'ASC, `t`.`menu_order`, `t`.`post_title`, `t`.`post_date` LIMIT 0' at line 14
jimtrue commented 8 years ago

In looking at the SQL, it's skipping passing the orderby field in its entirety in the PHP7 error.

sc0ttkclark commented 8 years ago

Shouldn't this be 'orderby' => 'order_alpha.meta_value'?

jimtrue commented 8 years ago

When we were testing this the other day, it's not required to add .meta_value; this the one we were fighting an edge case around, thinking it was the naming of the field (he had 'order' which was obviously an issue), this seems to be something specific to PHP7 but we'd need to test and verify.

tomherold commented 8 years ago

Yes, but it will also issue a sql error.

Sent from my iPhone

On Dec 19, 2015, at 4:14 PM, Scott Kingsley Clark notifications@github.com wrote:

Shouldn't this be 'orderby' => 'order_alpha.meta_value'?

— Reply to this email directly or view it on GitHub.

jimtrue commented 8 years ago

@tomherold can you share that SQL error on PHP7 with order_alpha.meta_value? Might help to track down the fault domain.

sc0ttkclark commented 8 years ago

Why PHP7? Does it work with PHP 5.x?

Hope it's not some weird regex PHP7 bug

tomherold commented 8 years ago

Sure, will be back in 20 minutes and add the report to the issue ticket.

Sent from my iPhone

On Dec 19, 2015, at 4:27 PM, Jim True notifications@github.com wrote:

@tomherold can you share that SQL error on PHP7 with order_alpha.meta_value? Might help to track down the fault domain.

— Reply to this email directly or view it on GitHub.

tomherold commented 8 years ago

Yes, works with php 5.x...

Sent from my iPhone

On Dec 19, 2015, at 4:28 PM, Scott Kingsley Clark notifications@github.com wrote:

Why PHP7? Does it work with PHP 5.x?

Hope it's not some weird regex PHP7 bug

— Reply to this email directly or view it on GitHub.

sc0ttkclark commented 8 years ago

WAT

jimtrue commented 8 years ago

I know, right? You'd think this would've been caught by unit tests, right?

tomherold commented 8 years ago

Scott, It may be related to Regex as I had an issue when I switched to php7.

preg_match_all( '/<h([1-3])(.*)>([^<]+)<\/h[1-3]>/iU', $content, $matches, PREG_SET_ORDER );

I had to add the 'U' parameter to make this work again...

tomherold commented 8 years ago

Here is the sql error for this test: 

$forces = pods('forces');
$params = array(
'orderby' => 'order_alpha.meta_value',
'limit' => 10,
);
$forces->find($params);
Database Error; SQL: SELECT DISTINCT `t`.* FROM `wp_posts` AS `t` LEFT JOIN `wp_postmeta` AS `order_alpha` ON `order_alpha`.`meta_key` = 'order_alpha' AND `order_alpha`.`post_id` = `t`.`ID` WHERE ( ( `t`.`post_status` IN ( "publish" ) ) AND ( `t`.`post_type` = "forces" ) ) ORDER BY , `t`.`menu_order`, `t`.`post_title`, `t`.`post_date` LIMIT 0, 10; Response: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ' `t`.`menu_order`, `t`.`post_title`, `t`.`post_date` LIMIT 0, 10' at line 14

Tested with php 5.6.16 and works ok.

sc0ttkclark commented 8 years ago

So with the 'U' added back, no SQL error with PHP 7?

tomherold commented 8 years ago

No, that was another issue in another code that I had when I switched from php 5.6 to php 7.0. Just mentioned this as it may has to do with the issue of 'orderby'.

tomherold commented 8 years ago

I am back running php 5.6 on my site until the issue is solved. By the way php7 executes about 3x faster than 5.6 on my site.

pglewis commented 8 years ago

@tomherold: would you be able to safely test a patch under PHP 7? I have something to try that is passing unit tests, so maybe worth a look.

pglewis commented 8 years ago

I'm gonna go with "Needs Testing". I'm unable to quickly verify if this fixes this issue or not; high time for me to get a PHP 7 dev environment going.

tomherold commented 8 years ago

I think it's better you do some testing with php7 and when you have a fix I am willing to test it on my website.

It's a simple routine, basically what I posted.

Sent from my iPhone

On Dec 20, 2015, at 12:14 PM, PG Lewis notifications@github.com wrote:

@tomherold: would you be able to safely test a patch under PHP 7? I have something to try that is passing unit tests, so maybe worth a look.

— Reply to this email directly or view it on GitHub.

tomherold commented 8 years ago

Is it possible to use WP syntax to build an alternative function? e.g.:

$q = new WP_Query( array( 'orderby' => array( 'order_alpha' => 'ASC' ) ) );

pglewis commented 8 years ago

The issue seems to be due to pattern and replacement params to preg_replace with non-zero keys. I have not found anything in the breaking-compatibility lists concerning that behavior so I'm currently leaning toward "seems like a PHP 7 bug". Code the illustrates the behavior, with no Pods code involved:

https://gist.github.com/pglewis/42c7a6e3da5e465793e0

pglewis commented 8 years ago

Also, since I don't think the keys are even needed here, should clear up simply by calling array_values on $find and $replace before passing them to preg_replace

pglewis commented 8 years ago

I've verified that the patch in #3299 fixes this issue on PHP7, need to check unit test results.

pglewis commented 8 years ago

Fixes the issue for me on a PHP 7 shared hosting environment (props @quasel) and passes unit tests. The patch is now in 2.x and ready for testing.

tomherold commented 8 years ago

That's great news. Looking forward to the next point release and have my site running again on php7. Appreciate the quick fix...

pglewis commented 8 years ago

Verified as a PHP 7 bug now as well: https://bugs.php.net/bug.php?id=71178

Way to bring us the weird ones, Tom!

The patch is in 2.x and will go out with 2.6.1, I'm closing this.

pglewis commented 8 years ago

For the sake of time travelers arriving in the future: this bug should be fixed in PHP and arrive in the next PHP 7 release.