Problem/Motivation

Follow up from #3574506: Sorting by Views result counter causes ORDER BY unknown from @alexpot

fix existing views as the view configuration is only fixed if you return to the table settings and press save.

Steps to reproduce

NA

Proposed resolution

Fix any existing config

Remaining tasks

Implement

User interface changes

NA

Introduced terminology

NA

API changes

NA

Data model changes

NA

Release notes snippet

NA

Issue fork drupal-3588188

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

smustgrave created an issue. See original summary.

kieran.cott made their first commit to this issue’s fork.

kieran.cott opened merge request !16077

kieran.cott’s picture

kieran.cott
Location Manila 🇵🇭
commented
Status: Active » Needs review

I've taken a pass at implementing this, I hope I've understood #3574506 correctly.

MR 16077 adds a Views config updater method that finds table displays with result counter fields and sets their stored table `sortable` setting to `FALSE`. Then adds a new Views post-update to run that updater for existing views.

Open to suggestions for improvement.

quietone’s picture

quietone commented
Title: Fix existing views with click sortable per #3574506 » Add post update to fix existing views with click sortable
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Looks good! But left a comment around the MR. Would check in gitlab 11.x branch for examples. Reason they're not on main is they've been removed already for D12

kieran.cott’s picture

kieran.cott
Location Manila 🇵🇭
commented
Status: Needs work » Needs review

Thanks @smustgrave, I've added CounterFieldTableSortableUpdateTest under Functional\Update which hopefully mirrors the example you mentioned in the MR.

dcam’s picture

dcam commented
Status: Needs review » Needs work

Thank you for your work to improve Drupal. I reviewed the MR and found several items that need to be addressed or explained. I left comments on the MR about them.

kieran.cott’s picture

kieran.cott
Location Manila 🇵🇭
commented
Status: Needs work » Needs review

Appreciate you taking the time to review @dcam, I've replied to your comments and made some changes.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

believe feedback for this one has been addressed. Thanks!

quietone’s picture

quietone commented

I've updated credit. Also, the title could be improved.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

One comment on the MR, the new method on ViewsConfigUpdater needs to handle the case when deprecation triggering is one (e.g. when it's called from ViewsHooks::viewPreSave().

sjpagan made their first commit to this issue’s fork.

sjpagan’s picture

sjpagan commented

updateTableSortableCounter() now triggers the deprecation when deprecations are enabled, guarded by triggeredDeprecations. Two saves of the same view emit one message.

The post update calls setDeprecationsEnabled(FALSE) before ConfigEntityUpdater, matching the other views post updates.

updateAll() calls the updater directly. processDisplayHandlers() reads the display array before the callback and writes that copy back, discarding the change.

Kernel test covers both states of the flag. The update path passes with 288 assertions and no deprecation from views.

Does this need a change record, or is the issue link in the deprecation message enough?

sjpagan’s picture

sjpagan commented
Status: Needs work » Needs review