Crud: [PROPOSAL] Deprecate setFromDb()

[tabacitu]

This comes as a reply to @jonphipps 's PR #1169, as a possible long-term alternative.

Why I'm unhappy with setFromDb():

1. Since generated CRUD panels use it, A LOT of people use it, though hard-core Backpack users know better and _do not_. It was meant to be something temporary, that only devs use until they customize their panels, or for super-simple use cases, but people have used it everywhere. So they go from building panels bottoms-up, to customizing what setFromDb() does automagically. And that's bad practice, IMHO.
2. It decides for itself what columns to show and in what way, but a lot of the times it's wrong.
3. It decides both for fields and for columns (this could be easily fixed).
4. It sets a false expectation for Backpack users - that generated panels should _just work_. Without customizations. Which is, well, false.
5. Because it's so widely used, and affects what columns/fields admins see, it is so freaking difficult to update it in any way, because we risk breaking people's apps. Every time we updated it I had to test it in 8+ projects, to make sure we're not screwing devs who actually keep their apps up-to-date.

Since this method can't "go places" exactly because of problem no (5), one approach I was considering was to retire setFromDb() altogether. We'd mark it as deprecated with the next opportunity, then remove it. And in its place we could (should):

• instruct devs to write their migrations correctly and completely from day 1;
• when generating the model, we use the db table to do our best to auto-populate things like $table, $fillable, $timestamps, $dates, $casts, and most importantly - relationships;
• if the model is correct and complete, when generating a CRUD panel:
  
  
  
  • the Request could already have the basic rules (not null, number, email, string size, etc);
  
  
  • the Controller could guess the columns and fields by actually writing the API code for them in the setup() method, instead of using setFromDb();
  
  

This way:

• it will be considerably faster to generate correct panels;
• we would have the ability to make the "guessing" work better, add features, stuff like that, without breaking people's apps; because they only use it when generating panels, not when they're running;
• it would make it even easier for people to just change a few things in the generated panel; like a column type; a field type; a default value; things like that; just add/change the existing generated code;

What do you guys think? I think this is a HUGE endeavour, but frankly it's the only long-term solution I see, that would allow us to continually improve the generated admin panels. Ideally we'd be able to get to near-perfect generated panels, eventually. But with setFromDb()... no such chance...

[lloy0076]

It sounds like an interesting idea - and I'd schedule this for a backpack 5 at least TBH.

[jonphipps]

It's a great idea to rethink how cruds are generated, but I think setFromDb() is worth keeping. It just needs to be a bit smarter and more transparent. It is after all totally optional if someone wants to build panels 'bottom-up'.

But it needs to make better choices depending on whether it's building list/show or edit/create and by making sure that the dbal schema-guessing can guess as well as it can. It also could grab more data from the model, like [casts] and [guarded] arrays. It could make some reasonable guesses about validation rules based on the database restrictions, and use the relationships defined in the model.

I don't think this needs to be a breaking change if the choices it makes are transparent and configurable. Someone who already has a CrudPanel built wouldn't need to see anything new or different because they're already (re)defining the fields in each view, and are likely to have their own validation rules setup if they care. The main issue would be fields that weren't defined by setFromDb() in the past but now are included by default. But this could be configurable, with a default that mirrors current behaviour.

Configuration is an area where it might be possible to improve the generator without breaking backwards compatibility. For instance field name-based hints, like https://crestapps.com/laravel-code-generator/docs/2.2#model-clean-fields.

BTW, I've already fixed label generation. These tests pass now (although I changed a few to make them Title case). And I made the field generation a bit smarter about booleans and numbers, although it still needs a default configuration option -- 'input_numbers_as_text = true' to not break backward compatibility.

[lloy0076]

What got me interested was the setFromDB in the first place - I don't disagree with @tabacitu's statements but without the wow of seeing it get most things right (or even most things wrong) I'm not sure I would have investigated Backpack/CRUD further.

[tabacitu]

Damn it guys.

You're right, of course... The UX would be pretty disappointing without setFromDb(). And yes, it should be up to the developer whether to _build bottoms-up vs top-down_. Ok then, it stays. We can focus on better generators then. If the Model is better generated, and the Request is better generated, then we'll have an easier time making setFromDb() smarter. Or generate controller code that's even smarter than setFromDb().

Thanks for keeping me in check. This is another place where collaboration shines - I get excited about a bad idea, you make me understand why it's bad. Thank you! :-)

Let's close this.

Cheers!

[jonphipps]

I think this is worth keeping open and just changing the title to ‘Refactor setFromDB().

Wanting to deprecating it is totally understandable as a response to point 5: “it’s so widely used that it’s very difficult to update”. My initial response was to suggest that we should explore ways to make significant incremental improvements that wouldn’t _necessarily_ result in breakage.

But the rest of your points are valid and it’s worth continuing to discuss how to improve setFromDB().