Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ACF validation of url field #703

Closed
asanti82 opened this issue Jun 13, 2019 · 12 comments
Closed

ACF validation of url field #703

asanti82 opened this issue Jun 13, 2019 · 12 comments
Labels
module: ACF Integration with ACF

Comments

@asanti82
Copy link

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 herrvigg added the plugin: others Concerns integration with other plugins label Jun 16, 2019
@herrvigg
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

herrvigg commented Jun 18, 2019

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
Copy link
Collaborator

herrvigg commented Jul 5, 2019

@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:

  • unregister all the original ACF validation (because it's based on strings and we have arrays),
  • register our own ones instead but call the original separately for each sub-item in the array.

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

@herrvigg
Copy link
Collaborator

herrvigg commented Jul 5, 2019

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
Copy link
Collaborator

herrvigg commented Jul 5, 2019

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
Copy link
Author

asanti82 commented Jul 5, 2019

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 herrvigg changed the title Issue with ACF url field ACF validation of url field Jul 7, 2019
@herrvigg
Copy link
Collaborator

herrvigg commented Jul 7, 2019

@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
Copy link
Collaborator

herrvigg commented Jul 7, 2019

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
Copy link
Collaborator

herrvigg commented Jul 12, 2019

@asanti82 it turns out this problem happened for every field, though the errors didn't appear (yet). See ticket #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.

@herrvigg herrvigg added module: ACF Integration with ACF and removed plugin: others Concerns integration with other plugins labels Dec 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: ACF Integration with ACF
Projects
None yet
Development

No branches or pull requests

2 participants