Jump to content

Here we go again...


CodingJungle
 Share

Go to solution Solved by bfarber,

Recommended Posts

1 hour ago, bfarber said:

Thanks, I've submitted a patch for review but I'll push off a fix for 4.5 given how low level the code is and how low priority and rare the issue itself is.

While we are looking at this code. Why don't you just use isset instead of in_array? Seems to me it would be lighter in terms of performance. 

Simply: if (isset($this->words[$k]))
instead of: if (\in_array($k, array_keys($this->words)))

Or am I missing some reason you use this approach, which to me seems like more overhead?

I found a blog post on this: http://maettig.com/1397246220

Quote

The result really looks how you think it should look. Searching for a key in a hash table needs constant time, no matter how big the array is. That's exactly why we have hash tables, right? On the other hand, searching for a value in an array needs dramatically more time even on relatively small arrays with only 100 elements. For an array with 10000 elements in_array is more than 100 times slower.

So never ever do in_array( $key, array_keys( $array ) ), that's just stupid.

And some stackoverflow answers https://stackoverflow.com/questions/13483219/what-is-faster-in-array-or-isset

I haven't looked any closer into this code, than just watching the video, but if $this->words contains all the language strings, then I would expect some performance improvement by changing to use isset or array_key_exists (if you need to account for that the value could be null). 

Edited by TSP
Link to comment
Share on other sites

15 hours ago, TSP said:

Why don't you just use isset instead of in_array?

They use isset for 2nd part (else statement)

		if ( \is_array( $key ) )
		{
			foreach( $key as $k )
			{
				if ( \in_array( $k, array_keys( $this->words ) ) )
				{
					$return[ $k ] = $this->words[ $k ];
				}

				..........
			}

			..........
		}
		else
		{
			if ( isset( $this->words[ $key ] ) )
			{
				return $this->words[ $key ];
			}

			..........
		}

If I’m not mistaken there was a topic (or bug report) about this.

In the previous versions the code looks like

		if ( is_array( $key ) )
		{
			foreach( $key as $k )
			{
				if ( in_array( $k, array_keys( $this->words ) ) )
				{
					$return[ $k ] = $this->words[ $k ];
				}
				..........
			}
			
			..........
		}
		else
		{
			if ( in_array( $key, array_keys( $this->words ) ) )
			{
				return $this->words[ $key ];
			}

			..........
		}

So IPS changed ELSE part

Link to comment
Share on other sites

21 hours ago, bfarber said:

Thanks, I've submitted a patch for review but I'll push off a fix for 4.5 given how low level the code is and how low priority and rare the issue itself is.

at least you didn't say it was a "niche" issue 🙂 or i would've  been forced to break out the 'momo' again, but thanks, its all i was asking for 🙂, to make you guys aware of it.

21 hours ago, TSP said:

While we are looking at this code. Why don't you just use isset instead of in_array? Seems to me it would be lighter in terms of performance. 

Simply: if (isset($this->words[$k]))
instead of: if (\in_array($k, array_keys($this->words)))

Or am I missing some reason you use this approach, which to me seems like more overhead?

I found a blog post on this: http://maettig.com/1397246220

And some stackoverflow answers https://stackoverflow.com/questions/13483219/what-is-faster-in-array-or-isset

I haven't looked any closer into this code, than just watching the video, but if $this->words contains all the language strings, then I would expect some performance improvement by changing to use isset or array_key_exists (if you need to account for that the value could be null). 

i wondered this myself, why they didn't use array_key_exists, i didn't mention it in the video how i would've done it differently. from a performance POV i am not sure which would be faster, but i would imagine it would be one of those premature optimizations we were talking about the other day in slack, cause Lang::get() isn't used very often, not as much as addToStack is. 

 

Edited by CodingJungle
Link to comment
Share on other sites

  • 3 months later...
2 minutes ago, bfarber said:

I can't even recall what this issue is because the video is 15 minutes and I cba to re-watch it again since I've noted the issue is fixed in 4.5.

 

Whoops\Exception\ErrorException thrown with message "Undefined index: 6c047061df4701e5d56a5c86b15d3d17_desc"

Stacktrace:
#7 Whoops\Exception\ErrorException in /home/michael/public_html/dev/system/Lang/Lang.php:656
#6 Whoops\Run:handleError in /home/michael/public_html/dev/system/Lang/Lang.php:656
#5 IPS\_Lang:get in /home/michael/public_html/dev/system/Helpers/Form/Form.php:463
#4 IPS\Helpers\_Form:customTemplate in /home/michael/public_html/dev/applications/core/modules/front/system/notifications.php:156
#3 IPS\core\modules\front\system\_notifications:options in /home/michael/public_html/dev/system/Dispatcher/Controller.php:85
#2 IPS\Dispatcher\_Controller:execute in /home/michael/public_html/dev/system/Dispatcher/Dispatcher.php:152
#1 IPS\_Dispatcher:run in /home/michael/public_html/dev/init.php:821
#0 IPS\toolbox_hook_Dispatcher:run in /home/michael/public_html/dev/index.php:13

2020-02-10_19-49.thumb.png.c941e312987237203cbb3f6b32dbef4d.png

it is basically a type coercion, using the third parameter on in_array forces an identical check instead of just a equals check. it is just aggravating after 6 months the bug persist.

Link to comment
Share on other sites

Whoops\Exception\ErrorException thrown with message "Method IPS\Helpers\Form::__toString() must not throw an exception, caught Whoops\Exception\ErrorException: Undefined index: 6dbef3c91911fb0a0a055257a3ccd7d8_desc"

Stacktrace:
#2 Whoops\Exception\ErrorException in /home/michael/public_html/dev/system/Theme/Theme.php:4316
#1 Whoops\Run:handleError in /home/michael/public_html/dev/dev/Whoops/Run.php:433
#0 Whoops\Run:handleShutdown in [internal]:0

2020-02-11_13-55.thumb.png.72a385355cf547177cee74a670b9ef23.png

i'm also hitting this error when editing member profiles in the acp

Link to comment
Share on other sites

  • 3 months later...
Whoops\Exception\ErrorException thrown with message "Method IPS\Helpers\Form::__toString() must not throw an exception, caught Whoops\Exception\ErrorException: Undefined index: developer_build_type_desc"

Stacktrace:
#2 Whoops\Exception\ErrorException in /tmp/theme_core_admin_global_globalTemplateWqIz0B:0
#1 Whoops\Run:handleError in /home/sites/dev/www/ips44/dev/Whoops/Run.php:433
#0 Whoops\Run:handleShutdown in [internal]:0

I got this today trying to get to the app download page.

Hadn't it been for @CodingJungle who made me aware of this issue, I could have wasted hours rolling back to older PHP versions, reinstalling, etc.

The patch for this must be pretty simple, as all I had to do was add ", TRUE" to the code, and that makes me wonder why you haven't included this is any of the 5 releases (not including silent patches) since this was marked as solved.

Link to comment
Share on other sites

 Share

  • Recently Browsing   0 members

    • No registered users viewing this page.
×
×
  • Create New...