DocumentAST public properties

[spawnia]

From @JasonTheAdams comment #974 (review)

The only thing that stands out to me is the usage of the DocumentAST class. It has three public array properties, and two setters. For the sake of protecting the integrity of those array elements, I would suggest making getters, setters, and isset methods for the three properties, and then making those properties private. This also allows you to reconcile the exceptions. You really don't want other code modifying the AST directly without a method abstraction.

[spawnia]

Having those array properties public saves writing a whole bunch of boilerplate code, since we need:

• Iteration
• Access by key
• Checking if a key is present
• Insertion by key

I can see the benefit in protecting unwanted access, but there is a very real cost in how much code has to be written to achieve basically the same as we have now.

[JasonTheAdams]

My suggestion would be to use the Iterator Design Pattern for this one.

So you would make TypesIterator, TypeExtensionsIterator, and DirectivesIterator classes (with a parent base class if it helps with DRY code). The $types, $typeExtensions, and $directives properties would be replaced by these. Code that iterates of the properties would still work, and adding a type would instead look like $documentAST->types->addType($newType).

This would protect the integrity of the AST, which is the core of this whole library. It also would allow for better error and exception handling for retrieving and storing the objects. In terms of Single Responsibility, it would allow the implementing code to remove the exceptions and focus purely on working with the types, leaving the Iterators to manage the collections and respective complexity.

Anyway, hope that helps and makes sense! I'll be working on GraphQL stuff in our platform again this month, and updating to the latest Lighthouse at that time. So I could probably take a stab at a PR at that time, if you're interested. If you're not, no worries, I just won't put in the time. 😃

[JasonTheAdams]

We could also use the ArrayAccess interface if interacting with the properties like arrays is important.

[JasonTheAdams]

(Processing.)

The real gain here is controlling how the AST is manipulated. Arrays are a carte blanche, full access system. If someone completely replaces or empties the array, there's nothing to be done. Worse yet, there's not really even a good way to trace the issue. We could effectively recreate objects as an array here, but that's no the point; the point is to be mindful of how access should work, and what errors should be thrown to be trace issues.

[spawnia]

What you are saying makes sense, i can see the benefits. Using ArrayObject and Iterable would allow adding safeguards for controlling exactly what can go in those properties.

Maybe you can do a POC without going all in, just to gauge how it changes the code around it that uses the DocumentAST?

[spawnia]

Usage of the DocumentAST has not been a source of bugs, so i don't really see a need to change it right now. Closing this for now, we can still improve upon this if needed.