inpsyde / wp-app-container

DI Container and related tools to be used at website level.
GNU General Public License v2.0
38 stars 2 forks source link

[Feature Request]: V3 question and possible documention improvements #15

Open remyvv opened 2 years ago

remyvv commented 2 years ago

Is your feature request related to a problem?

Not so much a problem, more of a collection of question/suggestions after having tested upgrading/migrating a project over to the v3 of wp-app-container.

Describe the desired solution

Answer questions and evaluate suggestions if they make sense for a future improvement day.

Describe the alternatives that you have considered

-

Additional context

Questions:

Possible documentation improvement tasks:

Code of Conduct

gmazzap commented 2 years ago

What is the preferred way of creating an instance of the App\App class? Currently there are 2 ways: app() and App::new(), with the latter only creating the object but the former actually storing the instance created.

App::new() is the class constructor. I tend to use static constructors because "normal" construct are side-effectful, can not be deprecated, require use of parenthesis to call a method right after creation ((new Thing)->doSomething() VS Thing::new()->doSomething()). Moreover, they don't work welll will immutability considering they can be called after the object is instantiated often re-writing properties ($thing->__construct($newProp)), even if this can be mitigated using the readonly modifier, but that is only PHP 8.1+ (see https://3v4l.org/kiqPC VS https://3v4l.org/rgPQm)

The app() function is instead a way to implement a singleton reachable for the outside. For services, we use the container to have singetons, but before the container is there we need a way to obtain a signgleto for the app, andt app() is one way. Of course, the app() function will need to call the App class constructor, which is App::new().

That is a recommendation, the package does not realy on app() function to work, but using it could be useful to make the website truly extensible.


With the default configuration, when adding a ExecutableModule that registers a action on init, the registered action will never run.

That is not necessarity true. The add_action that executes the booting is indeed executed on "init" hook, but with a predictable (and publicly accessible) priority (see https://github.com/inpsyde/wp-app-container/blob/3.x/src/AppStatus.php#L231 and https://github.com/inpsyde/wp-app-container/blob/3.x/src/AppStatus.php#L23)

So, if inside run there's an add_action to "init" it will work as long as its priority is higher than Inpsyde\App\AppStatus::BOOT_HOOK_PRIORITY.

We should consider documenting it, yes. Things that needs to happen on "init" might be called directly inside modules' run(), or using a very late priority, maybe PHP_INT_MAX just to be sure.

Also consider there's a pretty extensive documentation on the workflow that the App bootting workflow, see https://github.com/inpsyde/wp-app-container/tree/3.x#providers-workflow.

But all documentation should be revisited and split in shorter chapters in docs/ folder as per more recent Inpsyde practices.


Possible documentation improvement tasks:

As said above, the first thing to do will probably split exisiting documentation in chapers in a docs/ folder, like we have done for other packages.

And yes, the specifics of v3 are mostly missing from current README because currently it is mostly a copy from v2 documentation with some (but not all) needed adjustments.