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

findMany() combined with getPage() gives pages whose parents are (incorrectly) NullPage #1549

Closed
erikmh opened this issue Mar 27, 2022 · 7 comments

Comments

@erikmh
Copy link

erikmh commented Mar 27, 2022

Short description of the issue

Page instances created by findMany() followed by getPage() believe they have no parent.

Expected behavior

If one gets a set of pages with findMany() and then iterates through them, getPage() should yield page instances that are aware of their parents.

Actual behavior

If one gets a set of pages with findMany() and then iterates through them, getPage() yields page instances that are ignorant of their parents; their parents are instead NullPages.

Optional: Screenshots/Links that demonstrate the issue

PW forum user @adrian has shown the issue well. The first Tracy dump shows getPage() working properly inside a find() with page 1023 having a parent of '/'; the second Tracy dump shows getPage() yielding the same page that doesn’t know about its parent:

Optional: Suggestion for a possible fix

Forum user @kongondo has speculated that this may be a result of this commit, which deals with findRaw()->parent — though if I’m reading things correctly, those changes came in 3.0.193, whereas this behavior is new in 3.0.192.

Steps to reproduce the issue

  1. Use ProcessWire 3.0.192 or later.
  2. Create a page via the back-end.
  3. As in adrian’s example above, in home.php use findMany() with a selector that will include the page you created.
  4. Use getPage(0) to refer to the first page in the resulting array.
  5. Attempt to refer to its parent.

Setup/Environment

Server Details

Software Version
ProcessWire 3.0.192
PHP 8.0.17
Webserver Apache/2.4.51 (Debian)
MySQL Server 10.4.20-MariaDB-1:10.4.20+maria~buster-log
MySQL Client mysqlnd 8.0.17
Server Settings
Parameter Value
allow_url_fopen 1
max_execution_time 300 (changeable)
max_input_nesting_level 64
max_input_time 60
max_input_vars 10000
memory_limit 256M
post_max_size 100M
upload_max_filesize 100M
xdebug
xdebug.max_nesting_level
mod_rewrite 1
mod_security
EXIF Support 1
FreeType 1
GD Settings
Parameter Value
Version 2.2.5
GIF 1
JPG 1
PNG 1
WebP 1
iMagick Settings
Parameter Value
Version 6.9.10
GIF 1
JPG 1
PNG 1
SVG 1
PDF 1
WebP 1
Module Details
Module ClassName Version
AdminOnSteroids 2.0.21
BreadcrumbDropdowns 0.3.7
FieldtypeCombo 0.0.7
FieldtypeMultiplier 0.1.3
FieldtypeRepeaterMatrix 0.0.5
FieldtypeVerifiedURL 0.0.5
FileValidatorSvgSanitizer 0.0.5
InputfieldCombo 0.0.7
InputfieldMultiplier 0.1.1
InputfieldRepeaterMatrix 0.0.5
JquerySelectize 1.0.5
ProCache 4.0.3
ProcessHannaCode 0.3.0
ProcessPageListerPro 1.1.3
ProcessProCache 4.0.3
ProcessSetupPageName 2.1.4
ProcessWireAPI 0.0.4
ProcessWireUpgrade 0.1.1
ProcessWireUpgradeCheck 0.0.9
SearchEngine 0.30.5
TextformatterAutoLinks 0.0.5
TextformatterHannaCode 0.3.0
TextformatterTypographer 1.1.1
TracyDebugger 4.23.26
@erikmh
Copy link
Author

erikmh commented Mar 27, 2022

PW forum member @matjazp suggests the problem may have been introduced with the commit “Fix issue https://github.com/processwire/processwire-issues/issues/1495,” specifically with the unset at line 1176, which seems likely.

@adrianbj
Copy link

adrianbj commented Mar 27, 2022

Just a follow up to note that it isn't specifically limited to getPage(), just findMany()
image

@adrianbj
Copy link

adrianbj commented Mar 27, 2022

This might also be useful - it's definitely got something to do with specifically getting one of the pages from the array, via getPage(), first(), or last()

image

ryancramerdesign added a commit to processwire/processwire that referenced this issue Mar 28, 2022
@ryancramerdesign
Copy link
Member

Thanks @erikmh @adrianbj I've pushed a fix for this issue. Please let me know if you still observe any issues.

Note that I would recommend using findMany() only in cases where you will be iterating the $items result. The reason is that pages loaded by findMany() are not cached because the same PageArray has to cycle in a new chunk of pages (and clear the others from memory) according to where you are in the iteration, enabling you to iterate a result of any size. Every time you access a method on the $items result, like getPage(), first(), eq(), etc. it's got to load a fresh copy of that page. So a PageArray loaded by findMany() is great for a one time large result set iteration, but not great for plucking specific individual pages from it or further filtering, since it has a dynamic set of pages that are not kept in memory.

@adrianbj
Copy link

Looks good to me, thanks!

@erikmh
Copy link
Author

erikmh commented Mar 28, 2022

This is beyond excellent, @ryancramerdesign — I’m stunned by your speed in fixing this!

I’ve taken your two-line change and directly replicated it on my server (running 3.0.197), replaced my find() with findMany(), and can confirm that everything’s working exactly as expected. I also installed the current dev version from GitHub and confirmed it that way.

Thanks for your advice regarding the use of findMany(): that’s exactly how I’m using it. It’s currently averaging only about 5% faster than find() in the real world on my site; but as the site gets larger, I expect that difference will increase.

@erikmh erikmh closed this as completed Mar 28, 2022
@ryancramerdesign
Copy link
Member

@erikmh Thanks for testing it out and confirming it works. While looking into this, I did also find some optimizations I can make with this too so I'll be committing those later today or tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants