Default `$attached` to `true` to avoid BC break?

[ThomasLandauer]

Am I right that for the $attached argument of MailMimeParser::parse(), true is the best way for most people? Especially since you're saying at https://mail-mime-parser.org/upgrade-2.0:

When passing a string resource, a Psr7 stream is created out of it. Passing true/false as the second parameter will have no effect.

So why not change the function's signature to:

public function parse($resource, $attached = true)

... to avoid the BC break?

[zbateson]

It's more complicated than that, if you pass a file resource and don't attach it, you now have to wait till you're done with the returned Message (technically as long as everything's been read it's fine) before you close the handle.

It's preferable to 'attach' for that reason cause it'll be closed with the Message, but if your next line after parse is fclose, that'll cause me problems.

The BC break forces a decision on how to handle the change, which could break things for many people anyway if you don't change them. I should probably explain this whole thing better on the main readme though.

[ThomasLandauer]

But what about people who always pass a string?? => They have to change all their function calls, just to add a bogus bool, and in exchange they're getting zero improvement ;-)

Is the resource thing completely new or just improved?

• If new: I'd separate it into two functions:

  function parseResource(resource $resource, bool $attached)

  ... and change the other to parse(string $text)

• If improved: Well, separate them too, but explain it better ;-)

Bottom line: For the "resource" guys, it doesn't make much of a difference if they just have to add an argument or change the function name too. But for the "string" guys, it makes a huge difference if they have a BC break or not.

BTW: If you're asking me, PHP 5.x is dead long enough to bump the requirement to 7.x (=typed arguments). People not able to upgrade can still stay on your v1.

[zbateson]

But what about people who always pass a string?? => They have to change all their function calls, just to add a bogus bool, and in exchange they're getting zero improvement ;-)

Better an easy error to fix than something complicated like a file resource being closed too early and not realizing why there's an error parsing or how to fix it/what changed. Resources were always supported. I didn't want to introduce a second, different method and force an error. This seems reasonable to me.

Look at these guys, using a string, changed the call, no complaints: https://github.com/spatie/laravel-ray/commit/dcf3e9f4ea954ea431ff1e528ed4c53656d0a020 -- wonderful users :P

BTW: If you're asking me, PHP 5.x is dead long enough to bump the requirement to 7.x (=typed arguments). People not able to upgrade can still stay on your v1.

I support php 5.x because I'm a user of php 5.x and of my library. So are these guys (that are users of this library) for instance: https://github.com/salesagility/SuiteCRM

I try to support the default PHP version installed on maintained RHEL versions up to the end of extended support, which means I'll probably be dropping 5.4 and 5.5 as officially supported in the next couple months. It might become too much trouble at some point though, but for now it's not really a big deal.

[ThomasLandauer]

Oh my gosh, PHP 5.x and Laravel and Red Hat! ;-) I'm the ~exact opposite: Debian and PHP 8 and Symfony :-)

I didn't want to introduce a second, different method and force an error. This seems reasonable to me.

Um, but right now you're forcing an error anyway - it reads:

[ArgumentCountError] Too few arguments to function ZBateson\MailMimeParser\MailMimeParser::parse(), 1 passed

So the relevant question for me is: Force it upon 100% of users, or just 50%?

[zbateson]

There's big difference though, with ArgumentCountError it's easy and obvious where the error is, and exactly how to fix it. I don't mind breaking it that way for 50% of my users.

I have implementations on php5 and RedHat (none on Laravel though), I have others on different things too, it's all over the board with where I'm using it really.

[ThomasLandauer]

No, I can't see any difference ;-) ArgumentCountError only tells me what's wrong (in the same way as UnknownMethodCall would tell me). But to find out how to fix it, I have to check the docs in either case...

But never mind, in the meantime I've fixed my (single ;-) call anyway - so if you're not going to change it, you can close this issue :-)