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

fixes issue with type transformations #29353

Merged
merged 1 commit into from
Nov 14, 2020
Merged

fixes issue with type transformations #29353

merged 1 commit into from
Nov 14, 2020

Conversation

jigius
Copy link
Contributor

@jigius jigius commented Jun 1, 2020

Summary of Changes

Since PHP 7.{1-4} server's logs are polluted with notice messages PHP Notice: A non well formed numeric value encountered in %ROOT%/libraries/src/Profiler/Profiler.php on line 126.

The problem is wrong type casting (from string to float).

This patch is resolving this.

Testing Instructions

Looking for mentioned notices into server's logs after any pages have being requested via http proto.

Attention: notice level of messages has to been allowed!

Expected result

Mentioned notices is not found.

Actual result

Mentioned notices is found.

Documentation Changes Required

None

@bonzani
Copy link

bonzani commented Jun 1, 2020

I have tested this item ✅ successfully on 4f1d9f5


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/29353.

@HLeithner HLeithner added this to the Joomla! 3.9.20 milestone Jun 1, 2020
@HLeithner
Copy link
Member

I moved this to the next version and have an annotation.

With the new code we loose "+" sign for time and memory on output. I would suggest to precompile this values before adding this to the object and in the printf function.

Or evaluate why and if we need this * / 1000 manipulation.

@SharkyKZ
Copy link
Contributor

SharkyKZ commented Sep 4, 2020

I would suggest to precompile this values before adding this to the object and in the printf function.

Are you suggesting to change values stored in Joomla\Profiler\Profiler::$marks? I'm hesitant to do this against staging in case anyone actually uses these values as string. On the other hand, this could avoid some problems in existing code when upgrading to PHP 8 (e.g. like we currently have in Debug plugin). In any case we should totally do this at least in 4.0.

I can make the PR, just need to know for which branch.

@SharkyKZ
Copy link
Contributor

SharkyKZ commented Sep 4, 2020

Anyways, this PR is fine for now.

@SharkyKZ
Copy link
Contributor

SharkyKZ commented Sep 4, 2020

I have tested this item ✅ successfully on 4f1d9f5


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/29353.

@joomla-cms-bot joomla-cms-bot removed this from the Joomla! 3.9.22 milestone Sep 4, 2020
@SharkyKZ
Copy link
Contributor

SharkyKZ commented Sep 4, 2020

RTC.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/29353.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Sep 4, 2020
@SharkyKZ
Copy link
Contributor

SharkyKZ commented Sep 4, 2020

Actually we could fix this here properly, also without changing data type. @jigius are you still around?

@HLeithner
Copy link
Member

@SharkyKZ I mean the text in the $buffer before this PR we had a + (plus) sign if the value is positive, after this change it's missing. but this doesn't touch the $marks array

@SharkyKZ
Copy link
Contributor

This doesn't change buffer output because it's formatted below with sprintf() without plus sign anyways.

Anyways you might want to take a look at #30582 which solves this in a different way, also solving errors on PHP 8.

@Quy Quy removed this from the Joomla! 3.9.22 milestone Oct 6, 2020
@SharkyKZ SharkyKZ added the PHP 8.x PHP 8.x deprecated issues label Oct 12, 2020
@HLeithner HLeithner merged commit 423c8df into joomla:staging Nov 14, 2020
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Nov 14, 2020
@HLeithner
Copy link
Member

Thanks

@HLeithner HLeithner added this to the Joomla! 3.9.23 milestone Nov 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Information Required PHP 8.x PHP 8.x deprecated issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants