Add static method ActiveRecord::saveMultiple()

[WinterSilence]

Added to ActiveRecord because unable to create intermediate class StoreableModel between Model and BaseActiveRecord

Q	A
Is bugfix?	❌
New feature?	✔️
Breaks BC?	❌
Fixed issues

[bizley]

What value does it add to the core? This is just the shortcut and you can easily do it in your own app.

[WinterSilence]

@bizley you can say this about half methods of AR

[bizley]

I disagree and I would reject this addition but since I'm not a decision maker here I would like to hear opinion of others.

[WinterSilence]

@bizley disagree with what? For example, look validateMultiple(), doesn't remind you of anything?

[bizley]

I disagree about "half of methods" but yes, this example is a valid point.

[WinterSilence]

@bizley what's about getDb(), findBySql(), updateAll(), deleteAll(), find(), getTableSchema(), primaryKey(), attributes(), equals(), isTransactional()? ))

[terabytesoftw]

This type of function should not be inside the core, there are different approaches for handling it, so it should be part of the user's code.

[WinterSilence]

@terabytesoftw as validateMultiple()? (:

there are different approaches for handling it

for example?

[machour]

I don't see any added value, and if one of the models isn't saved, we won't know which one it is.
Better to leave this out of the framework.

[WinterSilence]

@machour

we won't know which one it is

if method return false, then you can display form with errors by ActiveForm, see manual example

"better" is implement pattern "collection", but Yii2 devs don't do it.. you don't do it..
Load and validate multiple models?
In framework
Save multiple models?

Better to leave this out of the framework.

I cant find any logic in this

[schmunk42]

As said before :) Yii 2 is actually in a feature freeze state.
It's a general questions if we want to add new public methods, IMHO this should be very limited.

And if we do so, we'd need extensive tests for it.

[WinterSilence]

@schmunk42 i don't try to add new features, I only complete existed features: added method missed in current implementation. adding tests in PR it's not problem.

[schmunk42]

added method missed in current implementation

In which way is this "missed"?

There's no implementation under the name saveMultiple somewhere else, just discussions.
https://github.com/yiisoft/yii2/search?q=saveMultiple&type=issues

[WinterSilence]

@schmunk42 if you can load and validate multiple model, then there must be a way to save them too. The framework should support it completely or not support it at all. https://www.yiiframework.com/doc/guide/2.0/en/input-tabular-input#updating-a-fixed-set-of-records look like "white spot" in model API.

[schmunk42]

Related discussions:

• Dealing with related AR models when loading, validating and saving #7585
• Load Related & Save Related [enhancement] #8565
• Provide BaseActiveRecord::saveMultiple() #8620

[WinterSilence]

@schmunk42 it does not explain why loadMultiple/validateMultiple is normal, but not saveMultiple.

[darkdef]

Sorry, but me don't like this enhancement.
Also i'm think updateAll, deleteAll - wrong examples. It's commands having alternatives in sql queries.
saveMultiple - it's example of typical business logic and this shouldn't be parts of framework. As well as the validateMultiple method. But removing it would be BC

[WinterSilence]

@darkdef

saveMultiple - it's example of typical business logic and this shouldn't be parts of framework.

*Multiple methods are abstract short-hands and not contains domain/business logic

Also i'm think updateAll, deleteAll - wrong examples. It's commands having alternatives in sql queries.

don't understand why they are wrong examples. updateAll() not contain extra logic, it's inline short-hand - you can call static::getDb()->createCommand()->update(static::tableName(), $attributes, $condition, $params)->execute(); directly

this shouldn't be parts of framework
As well as the validateMultiple method

they can be part of framework, but in other place (trait, behavior, Model collection)

But removing it would be BC

Is not abstract discuss about whether it's right or wrong way. It's conversation about whether it's right for current implementation: if class have loadMultiple() and validateMultiple(), then saveMultiple() look like logical solution.

*Multiple methods added to package by his owners (as you). Owners(you) could have moved these methods into separate behaviour/trait for long time, but they(you) didn't do this.

[rustamwin]

Note that the validateMultiple, loadMultiple methods are in the Model class, not in ActiveRecord.

[WinterSilence]

@rustamwin yes captain, but save() declared in BaseActiveRecord. Why saveMultiple() declared not in BaseActiveRecord? To replacing(not extending!) ActiveRecord by application (see Base* class pattern) i.e. it's optional feature.
Current ActiveRecord class declare&override methods i.e. violates Base* pattern(not discuss here)

[WinterSilence]

@bizley @terabytesoftw @machour @schmunk42 @darkdef Do you have any objections? In Yii2 context and not personal. Let's try give answer: why loadMultiple()/validateMultiple() is normal, but not saveMultiple()?
P.S. look to this as visit to doctor: nobody like it (doctor too), but it's necessary to normal work.

[machour]

I do have an objection, that I mentioned and you discarded, so I don't know why you're asking again 🤷🏼
And Yii2 is supposed to be in feature-freeze mode.

[WinterSilence]

@machour

In Yii2 context and not personal

I do have an objection, that I mentioned and you discarded

your objection NOT in Yii2 context and don't give answer on question:

why loadMultiple()/validateMultiple() is normal, but not saveMultiple()?

[machour]

And Yii2 is supposed to be in feature-freeze mode.

Because they were there before the feature freeze ?
Anyway, please don't mention me anymore to debate this :)

[darkdef]

It was wrong to add loadMultiple/validateMultiple to ActiveRecord domain. But now their removal is impossible without bc. My vote is against this change.
But It's great idea for refactoring of Yii3 active-record. Moving all of that's methods to separate class

[bizley]

Ok, looks like most of the team is against adding this method so we will not do it. Thank you all for your time and the input.

[WinterSilence]

@darkdef

But It's great idea for refactoring of Yii3 active-record. Moving all of that's methods to separate class

We can't do this, it's possible only in next major version

@machour the project is frozen when no one wants to support/update it. in current case it's not true. if you don't want change yii2, don't do it, but don't stop others from doing it.

@bizley reason to close?

[machour]

You are absolutely wrong.

The project is on feature-freeze, not because I don't want to change it, but by a team decision:
https://www.yiiframework.com/news/156/yii-2-1-and-yii-2-0-feature-freeze

Read Yii 3 instead of Yii 2.1 in the announcement.
It was decided to freeze feature so that the team can focus on the next version.

You are very welcome to submit new features to Yii 3.

You are also free to start a discussion about wether or not we un-freeze Yii 2, before bullying people into merging your PRs.

You are also free to fork the Yii2 framework and maintain your own fork with your own policy.