fix(browser): allow save source as raw content

[welcoMattic]

Fix: https://github.com/zenstruck/browser/issues/120

We can now save source as raw, and make assertions on file like zip or any other binary format.

NOTE: we could improve assertions on the zip file itself by requiring ext-zip and use \ZipArchive class to open zip.

[welcoMattic]

@kbond I agree, we can base the raw process on the content type. For json, as the content type is application/json the metadata should be added, I'll write a test for it

[kbond]

Sounds good! For the $raw parameter, you can either remove it entirely (can always add later) or make it ?bool $raw = null. When null, we make it true if content-type is text/* or application/json.

[nikophil]

not sure about the parameter if we implement the suggestion based on content type: if we have something like application/zip we'd never want the metadata to be added

[welcoMattic]

I've addressed all your comments @kbond @nikophil :wink:

[welcoMattic]

I rewrite the code to put the metadata prepended to the content under a condition via a global flag named BROWSER_SOURCE_DEBUG (set to false by default).

The only exception to this, is the call of source() method from dump() method, here we force the metadata to be prepended.

[welcoMattic]

I'm actually writing tests, and I expect to be able to compare files to saved sources from Browser (zip, html, jpg, json). As I need to compare saved sources to static files I have in my tests folder, I would like to disable completely the metadata prepended to the sources.

But, in another hand, I would like to enable those if I need to debug something.

@nikophil WDYT about this general flag?

[kbond]

I would like to disable completely the metadata prepended to the sources.

Got it, didn't realize you had a real use-case.

I'm sort of still thinking a ?bool $raw = null parameter opposed to a global config. Would that work for your use-case?

[kbond]

not sure about the parameter if we implement the suggestion based on content type: if we have something like application/zip we'd never want the metadata to be added

Agreed. What I am suggesting is to, by default, only add metadata to html,xml,plain-text/json content types.

[welcoMattic]

To make a decision, I think we must retrieve the original reason of the metadata presence? Was there a specific reason?

[kbond]

It's purpose is to make debugging x/html/json errors easier when:

1. Running $browser->dump()/dd() (shows the metadata in the console)
2. When using the PHPUnit extension to output files after a failure/error. You can view these files in your console/explorer or make available to view in your CI (github actions)

(2) above is I suppose the only hard reason to actually write the files with the metadata included. If you are calling ->saveSource() yourself, you likely do not want the metadata.

[welcoMattic]

As it's for failure/error cases, even in CI, I think the BROWSER_SOURCE_DEBUG env var is quite appropriate, no?

Set to false most of the time, and toggle it to true in case of failure/error, if you need to debug some saved source files?

[kbond]

Yes, that's a fair point. I still think any non-text/json content should never have metadata added as it corrupts the file.

But I'm fine rolling with this as is for now as, by default, this won't happen anymore.

[welcoMattic]

I see your point, so I can also add a condition (in addition to $sourceDebug) to make a check on content-type, to never add metadata except for a fixed whitelist of content-type (which could grow up through time), is it ok?

[kbond]

Sure, we'll have to be loose with the check I think: str_contains('text/', $contentType) || str_contains('json', $contentType)

[welcoMattic]

Finally, I've added 2 conditions to writing metadata : sourceDebug config flag (via env var) and checking response header to determine if is text content or not.

Beside, we decided to never prepend source content with metadata if PantherBrowser is used (as we can't check on response headers with Panther response).

[kbond]

Thanks @welcoMattic!