StringTagField triggers a record write
Closed this issue · 8 comments
I just noticed an issue in one of our onAfterWrite callbacks (where we prepare a flat version of the data object for Elasticsearch) and after some research found that the trigger is the $record->write(); call in the saveInto method of the StringTagField class (line 273 in the latest 2.8.0 version).
The problem is, that this triggers the records write method including the callbacks for onBeforeWrite, onAfterWrite etc. before all form fields had been processed. The end result is that the record is not fully updated in the onAfterWrite callback.
So my question is: Is the write call even required at that place? As far as my understanding goes, the saveInto method should only update the affected record on not actually write it to the database.
In a quick test without the write call, I could not find any negative side effect.
Is the write call even required at that place? As far as my understanding goes, the saveInto method should only update the affected record on not actually write it to the database.
I agree that the call to write() shouldn't be there. The problem we face now is that it has been there for >7 years according to git blame, so it's possible some people are relying on it being there. Some consideration will need to be taken as to whether to remove it (because it being there is technically a bug) or leave it there (because removing it could represent a breaking change).
Ok, I can see the problem 🙈 Then thanks for the feedback, I guess we will go with a simple fork for now. Would be appreciated if you could update the issue as soon as the decision has been made.
semver and "bugs as a feature" strikes again.
If we deem that behavior is a bug, then it is not a feature. We can be cautious and release those kinds of changes as minor releases instead of patch releases but we shouldn't prevent bugs being fixed because they "could be relied on". That's what changelogs are for.
Alternatively, if we are really worried, we can put the write behind a feature flag with a warning that the feature will be turned off in the next major version.
Or, of course, we just fix the bug in a new major release.
Yup that makes sense. I will tend to err on the side of being overly cautious for a while, while I get used to my role. I'm happy to take your lead in situations like this in the meantime.
@dkliemsch if you're willing to create a PR to fix this, I'd be happy to review it.
It's an issue that we always need to try to balance the rules of semver vs being pragmatic. This is something that comes up all the time, and often there's a question about whether it's a bug in the code or the docs, etc.
I think there is no hard or fast rule, we need to take it on a case-by-case basis on what we think is right, but my personal view is we shouldn't just let bugs linger because they could be features to someone. This is also made harder by our aversion to major releases, but in this instance I don't think a major release is a problem and that gives people a way to easily opt-in or -out of the feature depending on if it was critical to their workflow or not.
semver and "bugs as a feature" strikes again.
absolutely 😅 I prepared a simple PR (#208) using a config value to opt-out of the "feature", "bug" or whatever we call it.
I would agree that this should be completely removed in a future major release but i guess it's not necessary to create one just for this. So maybe this solution is more appropriate and prevents others from running into problems.
The 'feature-flag' PR has been merged, thanks @dkliemsch.
I agree with Dan that a new major version here is not a huge deal, more so that this is not a module that would have a hard upgrade path (compared e.g. to the CMS).
Nice, thanks for the quick feedback and merge!
Can you give a rough timeline on when to expect a new release?