Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Counting of items broken in combination of filter_tags with attribute_text #1412

Closed
4 tasks done
HaraldHubinger opened this issue Jun 26, 2020 · 11 comments
Closed
4 tasks done
Assignees
Labels
bug A bug! A bug! Fast, squish it!
Milestone

Comments

@HaraldHubinger
Copy link

HaraldHubinger commented Jun 26, 2020

Checklist before I submit this issue report

I confirm that:

My environment is:

(Please fill in the actual values from your environment - check Contao Manager or use composer show)

Key Value Comments
PHP version: 7.3.16
Contao version: 4.4.49
MetaModels version: 2.1.7 fine in 2.1.6, bug comes with 2.1.7 and persists in later versions
Installed MetaModels packages:
DCG version:

Issue description

Up to metamodels/core 2.1.6 a filter_tags on attribute_text correctly couns items.
From metamodels/core 2.1.7 items are not counted anymore.

Discussed the issue with @zonky2. It is introduced by 6efcdf7 and related to #1335
If the changes to BaseSimple.php are undone it works again.

Steps to reproduce

  1. Create attribute_text.
  2. Create filter_tags referring to attribute_text.
  3. Create frontend_filter which counts items.

Describe the behaviour of the application

Up to metamodels/core 2.1.6 items are counted.
From metamodels/core 2.1.7 items are not counted anymore.

Describe the expected behaviour of the application

Items should be counted.

@zonky2 zonky2 added the bug A bug! A bug! Fast, squish it! label Jun 26, 2020
@zonky2 zonky2 added this to the 2.2.0 milestone Jun 26, 2020
@zonky2
Copy link
Contributor

zonky2 commented Jun 26, 2020

@discordier Currently I have no idea how to work around the fix for MySQL 5.7 - you might have to pull the count separately

@HaraldHubinger
Copy link
Author

HaraldHubinger commented Jun 26, 2020

Proposed solution for BaseSimple.php lines 160 and following.

  1. In my opinion adaption of else clause was not needed. There was no violation of strict SQL standard there. Only first branch of if-clause is affected.

  2. Adapt first branch of if-clause as shown below by wrapping FIELD clause in an arbitrary aggregate. It looks ugly, but resolves the ambiguity tackled by this restriction of SQL and it works. I tested it in MariaDB. I unfortunately have no testing environment with mysql 5.7, but it should conform to SQL standard.
    Inspiration came from https://stackoverflow.com/questions/5818216/is-it-possible-to-use-order-by-column-not-in-the-group-by which was conceived for MSSQL.

     if ($idList) {                                                                                                                                                                                             
         $statement = $this->connection->createQueryBuilder()                                                                                                                                                   
             ->select($strCol . ', COUNT(' . $strCol . ') as mm_count')                                                                                                                                         
             ->from($this->getMetaModel()->getTableName())                                                                                                                                                      
             ->where('id IN (:ids)')                                                                                                                                                                            
             ->groupBy($strCol)                                                                                                                                                                                 
             ->orderBy('MIN(FIELD(id, :ids))')                                                                                                                                                                  
             ->setParameter('ids', $idList, Connection::PARAM_STR_ARRAY)                                                                                                                                        
             ->execute();                                                                                                                                                                                       
     } else {                                                                                                                                                                                                   
         $statement = $this->connection->createQueryBuilder()                                                                                                                                                   
             ->select($strCol . ', COUNT(' . $strCol . ') as mm_count')                                                                                                                                         
             ->from($this->getMetaModel()->getTableName())                                                                                                                                                      
             ->groupBy($strCol)                                                                                                                                                                                 
             ->orderBy($strCol)                                                                                                                                                                            
             ->execute();                                                                                                                                                                                       
     }
    

@MacKP
Copy link
Contributor

MacKP commented Jul 8, 2020

This fix works for me.
Thx @HaraldHubinger ;-)

@MacKP
Copy link
Contributor

MacKP commented Jul 9, 2020

Here some tests:

  1. New query:
explain SELECT city, COUNT(city) as mm_count FROM mm_objects GROUP BY city ORDER BY city ASC

1,SIMPLE,mm_objects,ALL,,,,,38,Using temporary; Using filesort
  1. Old query:
explain SELECT city, COUNT(city) as mm_count FROM mm_objects GROUP BY id, city ORDER BY city ASC

1,SIMPLE,mm_objects,ALL,,,,,38,Using temporary; Using filesort

Kind regards

stefanheimes added a commit that referenced this issue Aug 26, 2020
Fix for #1412

This should fix the problem: Counting of items broken in combination of filter_tags with attribute_text
stefanheimes added a commit that referenced this issue Aug 26, 2020
Hotfix release 2.1.10

- Fix for #1412 Counting of items broken in combination of filter_tags with attribute_text
- Do not add attributes if already present
@zonky2 zonky2 modified the milestones: 2.2.0, 2.1.9 Aug 30, 2020
@zonky2
Copy link
Contributor

zonky2 commented Aug 30, 2020

fixed with #1419

@zonky2 zonky2 closed this as completed Aug 30, 2020
@magicsepp
Copy link

fix doesn't work with contao 4.4.51 and MM2.1 see
6f299ad#commitcomment-41895022

@zonky2
Copy link
Contributor

zonky2 commented Aug 31, 2020

the fix work but is incompatible with sql_mode only_full_group_by
please open a new issue

@magicsepp
Copy link

magicsepp commented Sep 1, 2020

afak ONLY_FULL_GROUP_BY SQL mode is enabled by default .... ggrrrr

@zonky2
Copy link
Contributor

zonky2 commented Sep 1, 2020

please open a new issue

@zonky2
Copy link
Contributor

zonky2 commented Sep 4, 2020

@MacKP @HaraldHubinger @magicsepp
es wird einen Rückbau geben und die Count-Option wird in MM 2.2 deaktiviert - siehe #312 (comment)

@MacKP
Copy link
Contributor

MacKP commented Oct 2, 2020

Misst :( Aber Danke für die Info!

@MacKP MacKP mentioned this issue Mar 30, 2021
4 tasks
discordier pushed a commit that referenced this issue Apr 12, 2021
Hotfix release 2.1.0

- Fix for #1412 Counting of items broken in combination of filter_tags with attribute_text
- Do not add attributes if already present

Conflicts:
    src/Attribute/BaseSimple.php
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug! A bug! Fast, squish it!
Projects
None yet
Development

No branches or pull requests

5 participants