qtranslate / qtranslate-xt

qTranslate-XT (eXTended) - reviving qTranslate-X multilingual plugin for WordPress. A new community-driven plugin soon. Built-in modules for WooCommerce, ACF, slugs and others.
GNU General Public License v2.0
558 stars 107 forks source link

ACF validation of url field #703

Closed asanti82 closed 5 years ago

asanti82 commented 5 years ago

So not sure why it happens in server but not in my localhost. But in server I get the following message:

`Warning: strpos() expects parameter 1 to be string, array given in /home/alfredo/dm.alfredo.pe/wp-content/plugins/advanced-custom-fields-pro/includes/fields/class-acf-field-url.php on line 141

Warning: strpos() expects parameter 1 to be string, array given in /home/site/wp-content/plugins/advanced-custom-fields-pro/includes/fields/class-acf-field-url.php on line 141

Warning: Cannot modify header information - headers already sent by (output started at /home/site/wp-content/plugins/advanced-custom-fields-pro/includes/fields/class-acf-field-url.php:141) in /home/alfredo/dm.alfredo.pe/wp-includes/pluggable.php on line 1219`

I looked into the "class-acf-field-url.php" file and it has to do with the ACF url validation, it expects it to be a string to validate if it contains "://" that will make it a valid url or not, but I assume that the value it sends when using the qtranslate url it sends an array? Anybody else have this problem?

herrvigg commented 5 years ago

Do you still have the problem if you disable qTranslate-XT? Also it would be good to know who is calling this function. Do you have the full calling stack?

asanti82 commented 5 years ago

It only happens when I have an ACF Url translated field, if I use a regular ACF Url field I get no error. Not sure how to get the full calling stack, I copied everything that comes up when I hit save.

herrvigg commented 5 years ago

You can try by defining WP_DEBUG in your wp-config.php: https://wordpress.org/support/article/debugging-in-wordpress/

This should give you more context which is very useful to track the error.

Another tool i recommend is Sentry: https://sentry.io/. It requires a few more PHP lines for setup and the free account has some limitations but it is really powerful.

asanti82 commented 5 years ago

Well I just did the first part of enabling debug and logging it and all I got was:

[17-Jun-2019 23:17:24 UTC] PHP Warning: strpos() expects parameter 1 to be string, array given in /home/alfredo/dm.alfredo.pe/wp-content/plugins/advanced-custom-fields-pro/includes/fields/class-acf-field-url.php on line 141

But atleast it's letting me save the post without the headers modified issue so I can continue using it.

herrvigg commented 5 years ago

I don't really know how this works precisely, i'd need to install and test but i don't have time right now. The ACF module is directly derived from: https://github.com/funkjedi/acf-qtranslate. I don't think any changes in qTranslate-XT would cause a different behavior than the legacy stuff (combo qTranslate-X + plugin ACF-qTranslate). Maybe you could raise the issue there and see if other people encounter this.

herrvigg commented 5 years ago

@asanti82 All right, i could finally reproduce it and now i understand the problem. All the fields are handled as arrays in our integration code with one key per language, like this:

$value['en'] = 'my-english-url.com'
$value['fr'] = 'my-french-url.com'
$value['de'] = 'my-german-url.com'

But the original filters in ACF have no clue about this and treat those normally as string. This could lead to all kind of problems and i'm even surprised it could work until now. PHP is definitely not a hard-typed language... But for us it's quite convenient to mute the type to arrays for our internal processing and it should only be converted to a string when saved in DB (done with qtrans_join which is an old function btw).

Imo we should try to keep the array, even if we handle internally the values as encoded strings the validation for an URL would still be invalid.

But now regarding the validation, i think the right approach is to:

In that way we really handle the multiple values properly with the original ACF validation code.

herrvigg commented 5 years ago

If you want a very quick fix to avoid the warning, just unregister the QTX URL validation filter for now. I don't have the exact code right now but the tag filter is acf/validate_value/type=qtranslate_url.

herrvigg commented 5 years ago

I highly recommend to use xdebug with remote debugging, this is a life saver for Wordpress and PHP! I know managed to use that with PHPStorm with a server running in Docker and WSL (Linux on Windows), super convenient and it's working really good with decent performance. Waaay faster than MAMP for instance.

asanti82 commented 5 years ago

Woah I’m so glad you found it! And makes sense now why the error ocurrs. And yea Inwas just looking into xdebug, thanks!

herrvigg commented 5 years ago

@asanti82 fix now merged in master. No need to unregister/unregister. It was enough to override the validation function as it is handled by polymorphism.

herrvigg commented 5 years ago

Let me know if it works, but i'm confident as it was easy to validate including a special case for the required field that was not even used before. Note it will now stop as soon as one language is not valid (either required empty or invalid URL). It is like saying all the languages have to be valid.

Once validated this solution should be generalized to the other fields. There is definitely something missing here!

herrvigg commented 5 years ago

@asanti82 it turns out this problem happened for every field, though the errors didn't appear (yet). See ticket https://github.com/qtranslate/qtranslate-xt/issues/710.

I've found a generic method to handle all derived language fields, a bit cumbersome but it has the advantage of not duplicating the validation code and re-using the ACF now. It should also handle potential future changes out of the box. You're welcome to test all this.