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

Book module support: keys optional; bid can be new or FALSE; add original_bid, parent_depth_limit. #593

Merged
merged 1 commit into from
Jul 25, 2023

Conversation

mparker17
Copy link
Contributor

This is a follow-up to #523 - I know the book module is deprecated, but my project still uses it and we still want to use PHPStan to make our lives easier.

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 now aware of this as of #523 but:

  1. In practice, all of the properties in the $node-book array are optional;
  2. The bid property can be set to the string 'new', or the boolean FALSE when creating a new book (currently the rules only allow numeric-strings); and;
  3. The parent_depth_limit and original_bid keys are sometimes used inside the core book module (and could be used by code that takes inspiration from core).

Proposed changes

  1. Makes all the $node->book properties optional;
  2. Allows the bid property to be set to 'new' and FALSE;
  3. Adds the optional parent_depth_limit property (allowing ints and numeric-strings); and;
  4. Adds the optional original_bid property (allowing ints and numeric-strings).

Alternatives considered

I didn't really consider any alternatives, since the code already existed, it just needed to be changed slightly.

That being said, I did search core and ~84 contributed modules with the following regex to find any other edge cases missed...

('|")(nid|bid|pid|has_children|weight|depth|p1|p2|p3|p4|p5|p6|p7|p8|p9)('|")\s*?=>\s*?('|")(\P{C})('|")

... there are a lot of results (~29,000) in my codebase - most results in various test fixtures - but other than a handful of things unrelated to book (i.e.: establishing mappings), a scroll through results didn't show anything out of the ordinary... so hopefully I won't have to trouble you with more pull requests on this topic.

Testing steps

I did update the tests in this MR.

You could manually test by running php vendor/bin/phpstan web/core/modules/book when Drupal Core 9.5, 10.0, or 10.1 is installed, and looking through the errors. Note the book module sometimes sets EntityInterface::$book but making other entities into books isn't really possible.

* link_path: string,
* link_title: string,
* "nid"?: int|numeric-string,
* "bid"?: int|'new'|numeric-string|false,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Today I learned: you can provide valid string values to act as an enum!

@mglaman mglaman merged commit 0b93022 into mglaman:main Jul 25, 2023
@mglaman
Copy link
Owner

mglaman commented Jul 25, 2023

Thanks!

@mparker17 mparker17 deleted the book-module-improvements branch July 25, 2023 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants