Re-submitting #390

[jadb]

Refs: https://github.com/thephpleague/factory-muffin/pull/390#issuecomment-109944717

/cc @philsturgeon or @frankdejonge

[GrahamCampbell]

As you know, :-1: from me, because this package is designed for work with laravel's database component.

[jadb]

:+1: from me (fwiw) because this is the League of framework-agnostic packages?

[philsturgeon]

As discussed in #390, there are three situations.

1. This package is designed to work only with Laravel, and as such should be booted out of the League.
2. This package is designed to work only with Laravel, and should have Laravel specific code moved to a bridge package instead.
3. This package is designed to be entirely generic and just so happens to work with Laravel to the same level it works with any PHP framework or generic PHP, and as such we should remove the keyword.

I know it sounds like a lot of fuss about nothing to you, but a) it's a fundamental core principle of this group that code is entirely agnostic, and b) we don't want others feeling like we favor a specific framework.

When somebody raises a concern (which they did) I'd rather we convince others to understand their concerns, instead of me just coming over the top and doing it myself.

Buuuut I can see the merge button and ima click it. :trollface:

[GrahamCampbell]

Phil, please actually look at our code. It's not laravel specific. No framework specific code is needed for this work in a framework. I don't know why people are creating bridges that have like 3 lines of code in them.

[GrahamCampbell]

I know it sounds like a lot of fuss about nothing to you, but a) it's a fundamental core principle of this group that code is entirely agnostic, and b) we don't want others feeling like we favor a specific framework.

I agree here. I'd argue that "laravel packages" are framework agnostic though. At least, I always write them in that way.

[GrahamCampbell]

As I said already. It's not worth the argument. If the other league members want this change, then it's happening.

[philsturgeon]

@GrahamCampbell the conversation here is very circular and I am not sure why.

You say it's designed specifically to work with Laravel, then you say it is perfectly agnostic. It is one or the other.

I am saying: If its Laravel specific, then stop that. If it's not Laravel specific, it doesn't need to be advertised as such.

You can have your cake, and eat it, and pretend you didn't eat it, and tell other people it tastes good.

[philsturgeon]

This is not an argument.

[GrahamCampbell]

Gahhh. Incase you haven't already noticed, I'm shit at explaining things.

The reason I'm saying it works with laravel, is because we haven't got any tests to prove it works anywhere else, so by definition, we support laravel.

Phill, something can be agnostic and work fine in laravel. What's the problem? Laravel's not some magic place that suddenly you have to write special code for.

[GrahamCampbell]

https://github.com/thephpleague/factory-muffin/blob/master/tests/EloquentTest.php

[philsturgeon]

Progress :)

The reason I'm saying it works with laravel, is because we haven't got any tests to prove it works anywhere else

Step 1: make some tests that show it works outside of an Eloquent context. I'm sure the CakePHP folks will be happy to help. Right @jadb?

so by definition, we support laravel.

Yup, but listing one specific framework and not listing any others skews perception as to just how agnostic it is. You can list frameworks you support in the README, and if a bridge package is needed then make one (yes I Laravel doesn't need one, yes some frameworks might benefit from one, that's not the point). But just listing one is bogus.

Phill, something can be agnostic and work fine in laravel. What's the problem? Laravel's not some magic place that suddenly you have to write special code for.

Never said otherwise. Even factory_girl_rails is not necessary, but integration is tighter when it's used. If, when we add tests for non-eloquent-like packages, we find that some code could or should be moved out of the package, then that's where it would go: a bridge package.

Generally speaking though, Factory Muffin does not need to test that the ORM works. It needs to test that the object instances are generated, and that is it. Properties set on a class is not very Eloquent specific.

Does define() define stuff? Yes, yes it does. That is what we test, and at no point anywhere do we need to care about Laravel or Eloquent at all. :)

But that's all beyond the scope of this PR or the conversation being had. As long as you know that this not, should not and will not be Laravel specific, labeled as such, or thought of as such, we're onto a winner.

[GrahamCampbell]

Does define() define stuff? Yes, yes it does. That is what we test, and at no point anywhere do we need to care about Laravel or Eloquent at all. :)

Yeh, that may be true, but some of our other code isn't quite that simple, and it's useful to have integration tests because unit tests can lie.

[philsturgeon]

And you should totally keep them now they're there, I'm just saying you don't need to have CakePHP ORM tests, and SlimPHP tests, and whatever else tests because we're just setting properties on an object right meow and thats pretty simple.

[philsturgeon]

aaaaaand if you find any of that stuff being actually Eloquent specific then you should whip that out into another package called factory-muffin-eloquent and we're all happy campers.

[frankdejonge]

For reference, we have the tactician bundle which ties tactician together with Symfony adding service providers and the whole lot. The main package supplied the core functionality in a framework agnostic way. The bundle ensures it works correctly in Symfony. As an added benefit the specific framework package (bridging package) can also add version constraints, which makes the integration more reliable dependency-wise. A league/factory-muffin-laravel package could do the same and is perfectly welcome IMO. It could even have dedicated test cases to ensure compatibility. I've tackled it that way with Flysystem adapters too, even getting help from other people of adapter dependencies. This is how I see the way forward: Package + supporting/bridging packages.

Also, @GrahamCampbell: thanks for the responses, the merge, and being open for feedback.

[GrahamCampbell]

aaaaaand if you find any of that stuff being actually Eloquent specific then you should whip that out into another package called factory-muffin-eloquent and we're all happy campers.

We don't have anything specific.

A league/factory-muffin-laravel package could do the same and is perfectly welcome

As I said already. That just isn't needed. You don't need any special code to integrate this with any framework.

[philsturgeon]

Take notice of my use of the future tense. :)

[jadb]

Thanks for merging @GrahamCampbell. I appreciate.