PyPDF2 should not overwrite warnings.formatwarning.

[podloucky-init]

Hello,

PyPDF2 1.2.0 overwrites warnings.formatwarning with its own implementation (utils._formatwarning) in pdf.py line 74:

warnings.formatwarning = utils._formatwarning

Unfortunately this may cause severe side-effects if PyPDF2 is imported in a larger application. In our case the PyPDF2 implementation of formatwarning caused IndexErrors whenever a warning was raised somewhere else (and the filename argument was not to the formatter's liking).

Personally, I do not think that it is a good idea for a library to interfere with the global logging/warning infrastructure.

P.S.: Apart from this problem, we have been using PyPDF2 successfully for some time now. Nice piece of software!

[mstamy2]

Hello,

I'm assuming this issue is the same as #39. This does seem like a major flaw, so we'll make rewriting the warning system a priority. I'm not too familiar with how warning systems are implemented; should PyPDF2 just use the default system?

[podloucky-init]

Hi,
Thanks for the prompt reply!
Yes, this is the same problem. I am not too familiar with the warnings module but I think PyPDF2 should use the default warning system and formatter if possible. Some people might want to provide their own warning handling in their app so you probably cannot rely on having your implementation available.
The warnings.catch_warnings context manager should make it possible to override the global warnings.showwarning function in the context of the PyPDF2 module.

[CrimsonZen]

I ran into this issue the other day. In case you were entertaining the idea of a stopgap hotfix, you might be able to avoid logging errors by changing this line:
PyPDF2/utils.py:38

file = filename.replace("/","\\").rsplit("\\",1)[1] # find the file name

to

file = filename.replace("/","\\").rsplit("\\",1).pop() # find the file name

Obviously there are less hasty ways of dealing with the problem that are preferable, but this is the hotfix we have used on our deployments. "Nasty side-effects" is right - completely unrelated code that normally sends a warning can end up breaking warnings itself, obscuring the original warning and changing a silent failure to a loud one. On our Django application, we started getting 500's from Apache instead of a nice log and email from Django.

[mstamy2]

When pyPdf was created, it wasn't used within large applications - it had fewer features and less PDF compatibility. So, the custom warning overwrite was probably not a big deal. Now that PyPDF2's being used as a part of large applications, I can agree that the custom warning implementation is a problem. I'll work on reverting to the default system, though I need to become familiar with Python's warning/logging system.

[podloucky-init]

@CrimsonZen I can feel the pain. We had exactly the same problem, but somewhat nastier: Django tried to trigger DeprecationWarning while processing a response. This gave a nice HTTP status code of 200 with a 0-byte response.
We circumvented the problem by disabling PyPDF2's overwriting of the warnings.* functions.

@mstamy2 We use PyPDF2 in a very limited scope but it was (and probably still is) the most efficient tool for the job. I do not even consider this bug/misbehavior to be a big deal as it is easy to spot and can be circumvented.

[mstamy2]

@podloucky-init Thanks for the feedback. On my end, I deleted the overwrites for formatwarning and showwarning. I am not sure how useful the custom implementations even were. So, would it be best to remove the overwrites for all users?

[mstamy2]

The custom warning implementations have been removed. The potential issues (3 separate cases reported here) aren't worth the extra information they provide.

It would be possible to re-implement the features in a way that doesn't overwrite any methods of warnings (a subclass of warnings for PyPDF2 to use?)

[podloucky-init]

Sorry for the late reply: You could provide a formatter that formats warnings from PyPDF2. Users can then choose whether they want to use your formatter.
It might make sense to use a subclass of warnings.Warning for PyPDF2 warnings and let your formatter only handle those.

EDIT: sorry, clicked on close by mistake.

[mstamy2]

I re-implemented warnings with flag overwriteWarnings. It defaults to True, and utilizes the custom warning formatter as before. When False, it avoids overwriting warnings.showwarning.

The warnings print out code segments when overwriteWarnings is False; we should probably fix that in the future (it looks a bit confusing).

Anyway, I hope this resolves the issue successfully. I tried to create a subclass, but I'm not familiar with making subclasses out of modules (is that even possible?). Using a flag should be just as effective, as long as users are aware of it.

[CrimsonZen]

Strong Opinions Time: No library should ever assign to another external library. This is a huge violation of abstraction — let the warnings module handle warnings, and let PyPDF2 handle PDF's.

The warnings print out code segments when overwriteWarnings is False; we should probably fix that in the future (it looks a bit confusing).

It may be confusing to some, but it's the normal behavior. Just leave it out.

[mstamy2]

@CrimsonZen I think that the author of pyPdf felt justified in overwriting the warnings module because warnings.py seemingly advocates such an action (I don't have the module available right now, but it does mention something similar).

You're right, though, and I agree about it being a strong violation of abstraction/OOP in general. Perhaps warnings.py advocates some type of extension, not a direct overwrite, and pyPdf's author misinterpreted.

I'll go ahead and permanently remove the overwrites then. I was hesitant to do so at first because PyPDF2 aims to be a direct successor to pyPdf (adding/correcting features rather than removing), but it's probably for the best.

I'm also assuming that whatever features provided by the custom implementations of showwarning and formatwarning weren't overly useful. I think the intent was to

1. Provide alternative logging through warndest parameter
2. Add a little extra info to warnings.
   Maybe we can can maintain these features through other means.

Anyway, thanks for your input - I'll re-remove the warnings overwrite tomorrow (I'm currently on a mobile device).

[pavelbrych]

Hello,
we just run into the same problems as described here with overwritten showwarning method in larger project (interferes with SqlAlchemy). Constructor parameter for PdfFileReader() to turn custom implementation off does the trick, but unfortunately PdfFileReader() is instantiated also inside PdfFileMerger.merge() method and there is no option to turn custom showwarning method here. Would it be possible to add such option, if removing this code entirely is - for some reason - not preferred?

Thank you for great software, apart from this works nice for us.