search

dingo / api

A RESTful API package for the Laravel and Lumen frameworks.
BSD 3-Clause "New" or "Revised" License
9.33k stars 1.25k forks source link

More compatibility with Lumen #1762

Closed SpinyMan closed 3 years ago

SpinyMan commented 3 years ago

FormRequest affected

specialtactics commented 3 years ago

Hey @SpinyMan thanks

I realise it may look more neat, but can you please not abstract the form request to a trait, I think that will cause problems either now or down the track. Basically keep both files separate, even if the contents are the same.

SpinyMan commented 3 years ago

I realise it may look more neat, but can you please not abstract the form request to a trait, I think that will cause problems either now or down the track. Basically keep both files separate, even if the contents are the same.

Hi, Sorry, dude, but I disagree with you) The trait here is a good solution: if someday the Lumen and Laravel FormRequest became different you will always can create separated methods for each of them.

specialtactics commented 3 years ago

@SpinyMan You can disagree all you like, that's irrelevant - instead, try to actually understand the point I'm making. Like I said I understand it's more neat, and it would be a good solution if this was bespoke code that nobody relied upon.

However as we are actually concerned with support, I am not merging in this code as it will obviously break implementations which check for traits of a class and don't take recursion like this into account. In the open source world you will find many examples of things not perfectly neat, done so because they want to support other packages and versions.

In addition it's risky precisely for the reason you describe, that someone in the future changes this trait to do something specific in Laravel or Lumen, without realising the repercussions. Having two separate files, forces people to think about the repercussions of such changes. It is overall much better engineering design for this use case (open source project, requiring broad support, having many contributors).

So you can either change the code, or I'm closing this PR.

Also in addition to that, can you please run tests locally and clarify whether they are passing (the travis integration is broken atm, trying to get hold of project maintainer).

SpinyMan commented 3 years ago

OK, I just wanted to show the problem with FormRequest and Lumen... You can disapprove this PR and write your own that will resolve the issue. Ball is on your side, dude ;)

specialtactics commented 3 years ago

I can't easily test the lumen aspect of this because I don't use it myself, and as I mentioned the travis integration is broken.

If there is demand for it, I'm sure somehow will do it, I'm very open to it, I just need the two providers' formrequest classes to be separate.