Fixed price filter when search by non-numeric value

[sreichel]

Description (*)

Related Pull Requests

Fixed Issues (if relevant)

1. Fixes TypeError: Unsupported operand types: int * string #3135

Manual testing scenarios (*)

1. go to admin manage products
2. filter price by non-numeric value

Questions or comments

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All automated tests passed successfully (all builds are green)
• Add yourself to contributors list

[m-overlund]

Fixes the issue

The base branch was changed.

[luigifab]

I can now filter by price with "a" and "b".
Only products with a price of 0 are displayed, instead of an error.

[fballiano]

this PR has to be rebased on main

[sreichel]

I thought patches go to v19?

[fballiano]

patches will be backported from main to the v19 branch. 1.9.4.x is to be deleted as soon as all PRs are rebased: OpenMage/rfcs#10 (comment)

The base branch was changed.

[sreichel]

I know why @colinmollenhour reverted this ... :P

[addison74]

I checked initially the reported issue. I confirm after testing that this PR solves it. Not only for NULL value but for others values like strings.

@luigifab - Filtering for price from a to b I am getting "No records found."

[luigifab]

You're right no records found... with main branch, but I still get free products with my store. There may be other changes elsewhere.

[addison74]

@luigifab - There is no need to do other checks. I confirm your issue here #3158.

[sreichel]

@addison74 confirm this as intended. This PR was just to fix the error.

[addison74]

@sreichel - I approved this PR created by you precisely because it solves the previously reported issue (#3135). I confirmed a new issue and I created a ticket in this regard, so that another PR can solve this one as well.

[fballiano]

can't merge on this branch

[colinmollenhour]

I know why @colinmollenhour reverted this ... :P

Sorry, I used this PR as a guinnea pig to see if I could successfully rebase PRs of other authors - I can edit the base branch but it doesn't modify any commits so it just creates a huge mess.. AFAIK the only way to do it is manual rebase and then change the PR base as detailed here: OpenMage/rfcs#10 (comment)

[sreichel]

Githubs "squash merge" should do the same. Not? Just reverted base branch b/c off created mass.

[luigifab]

At localhost, I using git rebase -i main other_branch, then I remove lines of unneeded commits, then I change base branch on github, then I push -f.

[colinmollenhour]

Githubs "squash merge" should do the same. Not? Just reverted base branch off created mass.

The potential for conflicts that git cannot auto-fix safely between 1.9.4.x and main is very high, but there are also many commit differences between 1.9.4.x and 20.0 (the origin of main) so when you change the PR base it shows hundreds of additional commits.. Unfortuantely I think the manual rebase is the only option, I couldn't find any other way. Creating a new PR with cherry-pick would be another way but I think that just creates more clutter in the issue tracker and divides the conversation.

[colinmollenhour]

2023-04-12_15-35-26.mp4

[fballiano]

@colinmollenhour how do you push? cause I get a 403 when I try

[colinmollenhour]

@colinmollenhour how do you push? cause I get a 403 when I try

You had "mixed roles" so maybe that was the cause.. I removed the conflict favoring the more elevated role so maybe it will work now? You have to be a maintainer of some degree to push on other people's PRs and the author has to have to have selected "Allow edits and access to secrets by maintainers"

[sreichel]

@colinmollenhour how do you push? cause I get a 403 when I try

Mhh, you should be able to push?!? Thanks @colinmollenhour .

[fballiano]

@colinmollenhour thanks Colin, sadly didn't fix it:

fab@MBP-Fab ~/P/magento-lts (patch-8)> gpp
remote: Permission to loekvangool/magento-lts.git denied to fballiano.
fatal: unable to access 'https://github.com/loekvangool/magento-lts.git/': The requested URL returned error: 403

I think it's something regarding the way I authenticate on github but I can't find any doc about that :-\

[colinmollenhour]

I think it's something regarding the way I authenticate on github but I can't find any doc about that :-\

I always use SSH, not sure if that explains it.. I added the origin using the SSH url before pushing.

[sreichel]

@fballiano can please backport this?

[fballiano]

is this to be considered a significant regression or a vulnerability fix as per https://github.com/OpenMage/rfcs/blob/main/accepted/0002-release-schedule.md?

[sreichel]

Y, it is. Dont want to fix an error on admins catalog product grid?

[fballiano]

Thats not the point. Anyway you should have the privileges to cherry pick a commit to v19 branch

[addison74]

... but we cannot tell those who still use v19 to cherry pick individually PRs.

[fballiano]

What? Sven can cherry pick and commit it if he desires, not users. He is a maintainer.

[sreichel]

this PR has to be rebased on main

...

[fballiano]

you see, you keep reading everything in every post and just creating arguments. I've commented multiple times that I cannot (for some reason) push on other people's repo. so i cannot rebase it on main.

also, I'm more than willing to do that (if I could) for "normal users" but a maintainer (although you made it really clear we were not interested in collaborating with us anymore) can do it himself in 2 minutes.

[colinmollenhour]

@sreichel Fabrizio's comment saying it needed to be rebased on main was before it was merged. Anyway, I just cherry-picked it into v19.

In the future, after it has been merged into main if you want it backported please create a new PR for backporting (unless it is an urgent security matter then a maintainer will probably cherry-pick and backport immediately anyway). Perhaps we can use a label like "PATCH" to indicate if a PR should be backported at the time of merging..

But on that note, please observe that the RFC does state "functionality: if it doesn't fix a significant regression, it doesn't get a PATCH" - so I think it stretching a bit to justify a backport by that definition in this case. Let's try to adhere to the RFC more strictly going forward. Using the main branch is intended to be the way to take advantage of all of the various improvements whereas v19 is just on critical life support for those that are content with what they have and want minimal change. Of course there is always a third option which is to cherry-pick commits into your own repo to your liking, this is one reason we use Squash and Merge so that you can cherry-pick PRs easily. That's also useful for running unmerged fixes in production while we all argue about the minutia. 🤣

[colinmollenhour]

After just running into #3129 I understand this issue more now and it is definitely related to PHP 8+ which I somehow missed before - my apologies for overlooking this.

So this bug was not a regression so to speak but a result of support for a new PHP version - while this is not explicitly covered in the RFC I think it does make sense to treat bugs that are a result of forced PHP updates (7.4 is no longer supported so v19 must support at least PHP 8.0) the same as a regression and backport PATCH updates accordingly to ensure a supported version of PHP is always supported by supported versions of OpenMage.

In my opinion old MAJOR branches (e.g. v19) do not need to be updated to support bleeding edge PHP versions so when PHP 8.0 is EOL and PHP 8.3 is released on 26 Nov 2023, v19 should continue to receive updates to support PHP 8.1 and 8.2 (since it was already claimed to be supported at one point) but if PHP 8.3 requires significant updates these would not be considered PATCH updates so v19 will not support PHP 8.3.

As an aside, the single-command dev/openmage/install.sh dev environment was updated to support PHP 8.2 which is also the default.