Fix GridView generating unused label values (issue #19899).

[PowerGamer1]

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

[what-the-diff]

PR Summary

• Bug Fix in GridView
  This change addresses an issue in GridView, an element used to display and manipulate data in a tabular format. Earlier, GridView was unnecessarily using label values that were not needed. This bug fix ensures only the required label values are employed, improving the element efficiency.

• Configuration Change in ActiveDataProvider
  The sort configuration in ActiveDataProvider, the component responsible for fetching data, was unnecessarily using the 'label' attribute. This attribute has now been removed making the sort configuration cleaner and more streamlined.

• New Test File Addition
  A new file named NoAutoLabels.php has been added in the tests/data/ar directory. This indicates there have been some additional tests included to check certain criteria in the application.

• New Test Method in GridViewTest
  Adding a new test method called testHeaderLabels in GridViewTest signifies efforts towards ensuring the accuracy of the system. This method checks to confirm that GridView doesn't call the Model::generateAttributeLabel() function unless the labels are explicitly employed. This aids in maintaining the integrity and efficiency of our GridView system.

[codecov]

Codecov Report

Patch coverage: 33.33% and project coverage change: -30.21 ⚠️

Comparison is base (79c83ba) 48.39% compared to head (4539869) 18.19%.

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #19900       +/-   ##
===========================================
- Coverage   48.39%   18.19%   -30.21%     
===========================================
  Files         445      445               
  Lines       42302    42561      +259     
===========================================
- Hits        20474     7744    -12730     
- Misses      21828    34817    +12989     

Impacted Files	Coverage Δ
framework/data/Sort.php	16.96% <0.00%> (-72.86%)	⬇️
framework/data/ActiveDataProvider.php	72.58% <100.00%> (-8.67%)	⬇️

... and 265 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[rhertogh]

Are we sure this is not used?
https://github.com/yiisoft/yii2/blob/master/framework/data/Sort.php#L133-L134

[PowerGamer1]

Are we sure this is not used? https://github.com/yiisoft/yii2/blob/master/framework/data/Sort.php#L133-L134

Its fine. From your link:

If not set, [[Inflector::camel2words()]] will be called to get a label.

Which will happen at the moment when the label is actually needed here: https://github.com/yiisoft/yii2/blob/master/framework/data/Sort.php#L364-L366. That is if the Sort is used outside of GridView. When GridView renders label it will pass correct label value to Sort::link() in $options parameter.

[rob006]

Using Inflector::camel2words() when label is defined in attributeLabels() is not correct behavior.

[PowerGamer1]

Using Inflector::camel2words() when label is defined in attributeLabels() is not correct behavior.

That's not a problem of my fix but a bad engine design to begin with. For example, that exact "not correct behavior" you are referring to happens without my fix if you use ArrayDataProvider (instead of ActiveDataProvider) with array of \yii\base\Model objects with Sort.

[rob006]

That's not a problem of my fix but a bad engine design to begin with.

That is a regression created by your PR. So yes, it is a problem with your PR. You can question the design, by you can't ignore its limitations and break existing behavior because it does not match your particular use case. Generating labels when they're not needed is not perfect, but ignoring labels form attributeLabels() is way worse.

For example, that exact "not correct behavior" you are referring to happens without my fix if you use ArrayDataProvider (instead of ActiveDataProvider) with array of \yii\base\Model objects with Sort.

In that case we should improve ArrayDataProvider instead of braking ActiveDataProvider :)

[PowerGamer1]

That is a regression created by your PR.

I wouldn't be so sure. Calling "regression" a change in UNDOCUMENTED behaviour is stretching it - nothing in the doc for ActiveDataProvider suggests it will grab the labels from the model, same as for ArrayDataProvider. So I say my PR simply brings the behaviour of ActiveDataProvider on par with existing behaviour of ArrayDataProvider. Of course, that undocumented behaviuor of ArrayDataProvider (and ActiveDataProvider after this PR) may not be ideal, but that's a different story.

Generating labels when they're not needed is not perfect, but ignoring labels form attributeLabels() is way worse.

And this is where I strongly disagree.

The problem you referring to "ignoring labels form attributeLabels()":

1. Will happen ONLY when ActiveDataProvider with Sort is used without GridView.
2. Already happens with ArrayDataProvider.
3. CAN BE WORKED AROUND BY USER - just provide the correct 'label' when configuring Sort used without GridView.

What this PR fixes:

1. Creating EXTRA copies of the same label in memory (copying it from Model::getAttributeLabels() array into Sort::$attributes array).
2. Automatically generating labels with Model::generateAttributeLabel() that will be never used.
3. MOST IMPORTANT: allows the user to get rid of one of the stupid engine design decisions - explicitly concealing code ERRORs (missing labels) by automatically converting random internal variable names into UI labels. Without this PR, there is NO WORKAROUND FOR USER who raises error in Model::getAttributeLabels() for undefined labels (because he gets an error for an actually defined label, see my original issue report ActiveDataProvider generates label values which are never used #19899).

And unlike your problem my (multiple) problems happen when using the ActiveDataProvider WITH OR WITHOUT GridView. So I beg your pardon but I say that my problems are "way worse" than your problem.

In that case we should improve ArrayDataProvider instead of braking ActiveDataProvider :)

We fix multiple issues and remove some poorly implemented undocumented behaviour. After that, sure, implement label pulling from Model in Sort used without GridView (without introducing issues fixed in this PR) for both data providers. Personally, I would wait until someone requests that feature because currently there are much MUCH bigger issues in the engine needing attention.

[rob006]

Calling "regression" a change in UNDOCUMENTED behaviour is stretching it

This is not some random typo that created bug/feature, this is an intentional code to keep this behavior on pair with grid view or detail view (they use attribute labels from model too). Dismissing consistent and sensible behavior as undocumented - that is stretching.

Without this PR, there is NO WORKAROUND FOR USER who raises error in Model::getAttributeLabels() for undefined labels

Workaround is pretty simple: define all used labels in model. I believe the is preferred solution anyway - in most cases it is much easier to maintain labels in model than duplicating them everywhere separate for grid, sorting or detail view.

[PowerGamer1]

This is not some random typo that created bug/feature, this is an intentional code to keep this behavior on pair with grid view or detail view (they use attribute labels from model too). Dismissing consistent and sensible behavior as undocumented - that is stretching.

Yes this behavior is intentional but it is still poorly implemented and undocumented.

I believe the is preferred solution anyway - in most cases it is much easier to maintain labels in model than duplicating them everywhere separate for grid, sorting or detail view.

It's not. Because in practice a single model attribute has different labels depending on the context the label is used in (one label for forms, different for different grids, etc).

[rob006]

It's not. Because in practice a single model attribute has different labels depending on the context the label is used in (one label for forms, different for different grids, etc).

Nonetheless, defining generic label in model is simple and solves your problem from #19899, without the need of altering framework and removing an useful feature from it.

[PowerGamer1]

Nonetheless, defining generic label in model is simple and solves your problem from #19899, without the need of altering framework and removing an useful feature from it.

So having a choice of:

1. Lose some inconsistent, undocumented, poorly implemented behavior that only relevant in a rarely (if ever) used case - Sort without GridView. By doing that immediately gain performance benefits in a widely used case - Sort with GridView. Implement PROPERLY currently broken intended behaviour (once proven someone actually uses Sort without GridView).
2. Do exactly nothing.

you choose p.2. In that case suit yourself. Might as well freeze the framework in its current state and let it die already.

[rob006]

Lose some inconsistent, undocumented, poorly implemented behavior

Fetching attribute labels from model is consistent with grid and detail views - removing it would create inconsistency in framework behavior.
And I'm all for making ArrayDataProvider and ActiveDataProvider consistent by respecting attributes generateAttributeLabel() in ArrayDataProvider. If you will be able to avoid unnecessary calls to generateAttributeLabel() and keep current behavior, then great, go for it. But removing some feature because you don't need it and you don't like it, is not a solution for framework used by thousands of developers - probably someone is using it and this change may break his project.

[PowerGamer1]

Fetching attribute labels from model is consistent with grid and detail views - removing it would create inconsistency in framework behavior.

That's a two-edged sword. Since the behaviour is undocumented I can say that it is inconsistent with ArrayDataProvider and removing it will make it consistent with ArrayDataProvider.

But removing some feature because you don't need it and you don't like it

No, removing it because it comes with objective cons.

probably someone is using it and this change may break his project.

So what? In the past there were many breaking changes including those that broke my projects too. And among them a lot were unwarranted - with cons outweighing the pros. I don't recall the team so vigorously arguing against those back then. But, as I already said in previous posts, in this case the pros outweigh the cons so the change is actually warranted here. Actually, there will be no cons here - as soon as the currently broken behaviour is properly reimplemented.

Anyway, we are going in circles now. Fun fact: in the time you spend arguing with me you could have reimplemented that "oh so important" broken feature of yours properly. Can't do it? Ok, I'll do it for you as part of my fix. And as for you (and everyone else on the maintaining team) at least have a decency to TAKE A LOOK at INFINITELY MORE IMPORTANT problem in the engine here: #19788 and do something about it (like finding courage to revert BC breaking changes from the past responsible for it).

[rhertogh]

But removing some feature because you don't need it and you don't like it

No, removing it because it comes with objective cons.

But to weigh those cons more heavy than breaking backwards compatibility very much is.

probably someone is using it and this change may break his project.

So what? In the past there were many breaking changes including those that broke my projects too.

Because it happened in the past by accident, you now want to use that as an argument to do it consciously?
If there are good reasons to break BC then that should always be open for discussion, but this argument is a fallacy.

Other developers, including myself, have had pull requests rejected because it turned out that it would break BC, even if it would be more correct according to the documentation. But in general, the code is leading; Not the docs.

Based on your last comment you seem to agree that it's a BC breaking change? If so, please update the "Breaks BC?" row in the opening post.

Personal blaming won't resolve anything, so let's have a constructive dialogue on how to resolve the inconsistency. Perhaps it's something that can be fixed in Yii2.2?

[PowerGamer1]

So what? In the past there were many breaking changes including those that broke my projects too.

Because it happened in the past by accident

Yeah, right, "by accident". Every BC break I was speaking about (or the change that led to BC break) was INTENTIONAL and never reverted. Here is one for you: #19788.

But in general, the code is leading; Not the docs.

Says the maintainer of an engine for which the main selling point was ... the docs. What a sad state of affairs.

Based on your last comment you seem to agree that it's a BC breaking change? If so, please update the "Breaks BC?" row in the opening post.

As I said in my last comment, I already spend all those 5 mins to fix that badly implemented "feature" you so fond of (for which you prefer waste time to argue and defend instead of actually fixing) so now there are no longer any BC breaks (in undocumented behaviour or otherwise) in my PR.

Perhaps it's something that can be fixed in Yii2.2?

There better be a version of Yii soon that starts breaking BC left and right and won't stop doing it, or otherwise Yii is really dead. You may start with #19788.

[rob006]

It would be great to have tests covering usage of Sort::$modelClass, but overall solution looks good.

[samdark]

@PowerGamer1 have courage for @rob006 suggestion?

[PowerGamer1]

@PowerGamer1 have courage for @rob006 suggestion?

How about @rob006 finds courage to write tests for that EXISTING undocumented and untested behaviour (the one I had to fix for him) first? They will cover nicely code path in this PR using Sort::$modelClass, introduction of which was just a side effect from this PR point of view. The tests for exact thing this PR was aimed to fix are already included.

@samdark
You should find your own courage too - for #19788, which is still waiting for you.

[samdark]

Aha. I've re-checked the tests and these are enough.

[samdark]

Thanks!