Make EntityFieldReflection aware of the book property added by book.module.

[mparker17]

Problem / motivation

As of Drupal 9.5, 10.0, and 10.1, the book module adds a "book" property to Node instances that have been enabled as books.

PHPStan is not aware of this and thus reports messages like Property Drupal\node\NodeInterface::$book (Drupal\Core\Field\FieldItemListInterface) does not accept array.

Proposed resolution

Tell PHPStan to expect a book property on Nodes that is an array.

Additional notes

I wanted to be more specific about what's in this array, but I'm not certain how to do that in PHPStan - its API documentation is somewhat minimal. If you have suggestions, I would be happy to update this merge request.

The book property is an array that contains the following keys/types:

Array key	Data type
nid	int (or numeric-string)
bid	int (or numeric-string)
pid	int (or numeric-string)
has_children	int (or numeric-string or bool)
weight	int (or numeric-string)
depth	int (or numeric-string)
p1	int (or numeric-string)
p2	int (or numeric-string)
p3	int (or numeric-string)
p4	int (or numeric-string)
p5	int (or numeric-string)
p6	int (or numeric-string)
p7	int (or numeric-string)
p8	int (or numeric-string)
p9	int (or numeric-string)
link_path	string
link_title	string

... See book_schema() in 10.1.x core for more info. While many of these fields are stored as integers in the database, the book module usually queries them and dumps the query result into the array, and due to the way the database layer works, they come out as strings containing the integers that were stored in the database.

[mparker17]

At time-of-writing, the CircleCI Pipeline is failing with "Could not find a usable config.yml, you may have revoked the CircleCI OAuth app." - my gut says this is an upstream issue?

[mglaman]

CircleCI - thanks for heads up, I'll need to see why that broke.

Book module - I'm guessing this isn't a computed base field but a generic property set like original. Based on a quick mobile review this makes sense. There isn't a better way to handle it, honestly.

But we could possibly also fix this via a stub which uses PHPStan's array phpDoc syntax.

Something like

@property array{foo: string} $book

This could live on the NodeInterface

[mparker17]

@mglaman thanks for the quick review!

Book module - I'm guessing this isn't a computed base field but a generic property set like original. Based on a quick mobile review this makes sense. There isn't a better way to handle it, honestly.

That is correct: it's just a generic property set like $original.

But we could possibly also fix this via a stub which uses PHPStan's array phpDoc syntax.

Something like

@property array{foo: string} $book

This could live on the NodeInterface

I actually tried this before submitting this merge request, i.e.: I tried writing my own Node stub file in my own project, modifying the stub files in this project, and even adding @property declarations to Node in Drupal core, and nothing seemed to work (although in retrospect, while I do remember trying this on Node, I don't remember if I tried this on NodeInterface).

My impression was that the code in mglaman\PHPStanDrupal\Reflection\EntityFieldReflection::getReadableType() and ::getWritableType() took precedence, for whatever reason.

But if you'd like, I can try the stub file approach once more, modifying NodeInterface explicitly.

[mglaman]

You're right. I think another issue captured these reflection helpers override stubs

[mparker17]

You're right. I think another issue captured these reflection helpers override stubs

👍

---

Forgive my confusion, but I'm not sure what you'd like me to do next (if anything)! Thanks for your patience with me!

[mglaman]

Oh nothing else needs to be done. I'll review the PR this week

[mglaman]

I think we need isSubclassOf so that we can account for concrete class of Node and bundle classes.

Also, we should add a test. I can work on that if you'd like.

[mglaman]

Sorry! I forgot about this, I'll make my way back to it this week.

[mglaman]

I have one concern: there are discussions on moving Book to contrib; see https://www.drupal.org/project/ideas/issues/3265493.

This extension would then hold on to custom property reflections for just one contrib when many may manipulate node entities this way. Considering this while giving second review

[mglaman]

Okay! I fixed the issue of parsing @property on entity stubs, so I moved the book property types to the stub which seemed less hacky to maintain and makes it easier to eventually remove any funky entity field reflection code