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

Ignore @iconv notice messages #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pupaxxo
Copy link

@pupaxxo pupaxxo commented Dec 20, 2019

No description provided.

@zbateson
Copy link
Owner

I'm torn on this one -- on the one hand, the notices are annoying, but on the other hand if there's an actual error being reported in the call to iconv that would suppress it.

iconv is still an annoying function overall, but I'm not convinced this is a good idea particularly because both the mb_convert_encoding and iconv calls can fail if the charset isn't supported. What are your thoughts on suppressing those errors?

@pupaxxo
Copy link
Author

pupaxxo commented Dec 28, 2019

My idea was to suppress iconv notices, they are generally not so much useful, I didn't think about the errors thought... We can catch the errors using the iconv return value, from the PHP documentation it says that iconv return false on failure. We could check for failure and trigger the error only if we are not able to successfully decode it? What do you think?

@zbateson
Copy link
Owner

zbateson commented Jan 2, 2020

The trouble is catching it means I have to decide on some form of error handling/reporting which I've completely avoided... do I:

  • call trigger_error?
  • throw an exception? I've avoided this everywhere else, so feels out of place
  • log to a psr log? I've also avoided specifically writing logs, although had been thinking of adding that in at a future date to help people with debugging etc since I don't follow either of the above patterns

The other problem is because I've caught the error, I wouldn't have a meaningful message to pass on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants