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

fix: UpdateOnlyTextField incompatibility with DependConstraint #7879

Merged
merged 16 commits into from
Oct 12, 2024

Conversation

kumy
Copy link
Contributor

@kumy kumy commented Sep 21, 2024

Closes: #7878

  • create a public getter for accessing field current value
  • use the getter in BaseConstraint check

Fix applied manually on my test VM and work as I expect :)

@AdSchellevis is it what you expected? Is there some unit tests to run/adjust? Do you think of any other place the same change should happen?

@AdSchellevis
Copy link
Member

@kumy that's probably part of the solution, only issue is more constraints use (string)x which would suffer from the same issue as this one. For example:

if (!$node->isRequired() && empty((string)$node)) {

@kumy
Copy link
Contributor Author

kumy commented Sep 21, 2024

@AdSchellevis I've found potential other places. Please double-check 🙏 (new commits are untested sorry)

I'm also confused by a code part, please see my comment in code

@AdSchellevis
Copy link
Member

@kumy left some notes, but I like the direction this is going. Nice work.

@kumy kumy force-pushed the issue-7878 branch 2 times, most recently from 8c1a704 to 2bdbbf3 Compare September 22, 2024 08:20
@kumy kumy force-pushed the issue-7878 branch 2 times, most recently from dab0e8b to 1651ff8 Compare September 23, 2024 16:59
@AdSchellevis
Copy link
Member

@kumy after a couple of busy weeks, I thought to take another look at this. Looking at the code, I think it's mostly ready to merge, I'm just not sure about the naming of isEmptyButZero, how about isEmptyString? Ideally the isEmpty should be more context aware (boolean expects 0, string ''), but given the risk of regressions, that's not a valid path at the moment.

@kumy
Copy link
Contributor Author

kumy commented Oct 12, 2024

Thanks! Ok I will rename isEmptyButZero to isEmptyString

Ideally the isEmpty should be more context aware (boolean expects 0, string ''), but given the risk of regressions, that's not a valid path at the moment.

Agree, but such change should be separated from this PR 🙂

@kumy
Copy link
Contributor Author

kumy commented Oct 12, 2024

@AdSchellevis Thanks for the review 🤓

  • PR rebased
  • function isEmptyButZero renamed to isEmptyString
  • Unit Tests still passing
# phpunit --configuration PHPunit.xml 
PHPUnit 9.6.20 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.2.23
Configuration: PHPunit.xml

...............................................................  63 / 219 ( 28%)
............................................................... 126 / 219 ( 57%)
............................................................... 189 / 219 ( 86%)
..............................                                  219 / 219 (100%)

Time: 00:00.139, Memory: 50.88 MB

OK (219 tests, 456 assertions)

BTW, if the PHPunit.xml file name is all lowercase then it is automatically picked by phpunit

# cd /usr/core/src/opnsense/mvc/tests

# phpunit
PHPUnit 9.6.20 by Sebastian Bergmann and contributors.

Usage:
  phpunit [options] UnitTest.php
  phpunit [options] <directory>

[…]

# mv PHPunit.xml phpunit.xml
# phpunit
PHPUnit 9.6.20 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.2.23
Configuration: /usr/core/src/opnsense/mvc/tests/phpunit.xml

...............................................................  63 / 219 ( 28%)
............................................................... 126 / 219 ( 57%)
............................................................... 189 / 219 ( 86%)
..............................                                  219 / 219 (100%)

Time: 00:00.138, Memory: 50.88 MB

OK (219 tests, 456 assertions)

@AdSchellevis
Copy link
Member

@kumy not sure why I made it uppercase back in 2016, but if the default is lowercase, I don't mind changing that. Thanks for taking the time to finish this PR

@AdSchellevis AdSchellevis merged commit 8572171 into opnsense:master Oct 12, 2024
@kumy kumy deleted the issue-7878 branch October 12, 2024 15:10
@kumy
Copy link
Contributor Author

kumy commented Oct 12, 2024

Thanks! 🕺

not sure why I made it uppercase back in 2016, but if the default is lowercase, I don't mind changing that.

If you want to change it, I let you do it - it'll probably be easier like that. I bet the "uppercase" one lives in many places/repositories...

Any chance you have a bit of time for the other ones?

Thanks

@AdSchellevis
Copy link
Member

renamed the phpunit file in 84437b3

It looks like xymon-client is not very big and seems to build without issues on my end, I've added it to the build (opnsense/tools@88e547d) assuming @fichtner is ok with it since it builds on my end.

It has been pretty busy lately, so it probably slipped our attention.

fichtner pushed a commit that referenced this pull request Nov 5, 2024
…7879)

(cherry picked from commit 8572171)
(cherry picked from commit 17270c4)
(cherry picked from commit 96a37c2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

UpdateOnlyTextField is not compatible with DependConstraint
3 participants