samuelet / indexmenu

A dokuwiki plugin to show a customizable and sortable index for a namespace.
http://dokuwiki.org/plugin:indexmenu
GNU General Public License v2.0
44 stars 43 forks source link

#260: check before accessing #261

Closed BS666 closed 1 year ago

BS666 commented 1 year ago

Preventing undefined key warnings in php8 mentioned in #260 and #257

eduardomozart commented 1 year ago

Not sure if it's related with this PR, but after applying it, my index tree seems to be rendering incorrectly.

The following tree:

.
└── syspass/
    ├── plugin/
    │   └── authenticator
    ├── search
    ├── account
    ├── users
    ├── typology
    ├── audit
    └── config

Seems to be rendered as:

image

The pages appears to be linked to the "sysPass > Plugins" subfolder instead to be linked at the "sysPass" tree.

eduardomozart commented 1 year ago

It's also start throwing the following error after save page edits:

`` Fatal error: Maximum execution time of 90 seconds exceeded in /home/robertinho/public_html/wiki/lib/plugins/indexmenu/syntax/indexmenu.php on line 530 dokuwiki\Exception\FatalException: Maximum execution time of 90 seconds exceeded An unforeseen error has occured. This is most likely a bug somewhere. It might be a problem in the indexmenu plugin.

More info has been written to the DokuWiki error log. ``

image

There's no useful information at DokuWiki log:

2023-02-01 00:10:04
dokuwiki\Exception\FatalException: Maximum execution time of 90 seconds exceeded
/home/robertinho/public_html/wiki/lib/plugins/indexmenu/syntax/indexmenu.php(529)
#0 [internal function]: dokuwiki\ErrorHandler::fatalShutdown()
#1 {main}
BS666 commented 1 year ago

Hi, strange - I cannot reproduce the described behavior. Got a tree till fourth depth and no problem so far. Current version for me is "2022-07-31a". Did you apply more manual code changes?

eduardomozart commented 1 year ago

I'm also running the latest DokuWiki Igor "2022-07-31a" release and using the version downloaded from Extension Manager. I'd applied your fix and this issue occurs. I'm using PHP 8.1. I even attempt to use another template (Read the Dokus), but the timeout problem is still there.

eduardomozart commented 1 year ago

I believe I figure it out the issue related to the timeout at your code. The $prev variable didn't decrease so it never exists the "while" loop. You need to use the "end($q) - 1" directly like the original code did. Here's a working code without timeout issues:

-           $prev = end($q) - 1;
            while(isset($data[end($q) - 1]) && $item['level'] <= $data[end($q) - 1]['level']) {
                array_pop($q);
            }
LMS235 commented 1 year ago

Tested https://github.com/samuelet/indexmenu/pull/261/files => Wiki then no longer usable, timeout error (Maximum execution time of 180 seconds exceeded)

eduardomozart commented 1 year ago

I believe I figure it out the issue related to the timeout at your code. The $prev variable didn't decrease so it never exists the "while" loop. You need to use the "end($q) - 1" directly like the original code did. Here's a working code without timeout issues:

-           $prev = end($q) - 1;
            while(isset($data[end($q) - 1]) && $item['level'] <= $data[end($q) - 1]['level']) {
                array_pop($q);
            }

@LMS235 Please apply this patch and try again. I already notified the PR author but he couldn’t reproduce the issue that we both have, but this timeout happens because he accidentally created a loop that never ends because the “prev” variable always assumes the same value so the condition to end the loop is never meet. The code above fixes the issue. If this PR be merged, it should include the fix above.

LMS235 commented 1 year ago

@eduardomozart with your "patch" it works and no php warnings occour.

@BS666 / @samuelet the commit only works with the patch, otherwise there will be timeouts

nerun commented 1 year ago

I agree. Tested here and it does need @eduardomozart's patch.

samuelet commented 1 year ago

Before merging, it needs to be tested for backward compatibility. Feedback about running it in php7 env is welcome.

eduardomozart commented 1 year ago

Hello @samuelet, it's great to see you here! I'm able to confirm it's working as expected with this fix on PHP 7.4. But please notice that this PR as it is, it doesn't work as expected. You need to change the following, as already confirmed by @LMS235 and @nerun:

I believe I figure it out the issue related to the timeout at your code. The $prev variable didn't decrease so it never exists the "while" loop. You need to use the "end($q) - 1" directly like the original code did. Here's a working code without timeout issues:

-           $prev = end($q) - 1;
            while(isset($data[end($q) - 1]) && $item['level'] <= $data[end($q) - 1]['level']) {
                array_pop($q);
            }
samuelet commented 1 year ago

@eduardomozart i reviewed the pull request following your comment, please could you check the patch is now ok and can be committed ?

eduardomozart commented 1 year ago

Hello @samuelet, now it's exactly how it's working into my PHP 7.4 and PHP 8.2 DokuWiki environments without issues.