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

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

Open
1 task done
remyvv opened this issue Oct 31, 2022 · 1 comment
Open
1 task done
Labels
enhancement New feature or request

Comments

@remyvv
Copy link

remyvv commented Oct 31, 2022

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:

  • 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.
  • With the default configuration, when adding a ExecutableModule that registers a action on init, the registered action will never run.
    This is caused because the default for the AppStatus::$lastStepHook is init.
    We should consider clarifying this difference from how Modularity normally executes these modules in the docs.

Possible documentation improvement tasks:

  • Update examples & explanation in the README of the repository
  • Create section in README (or seperate file) that explains changes since v2
  • Create section explaining usage with modularity packages (App::addPackage v.s. App::addProvidersPackage)

Code of Conduct

  • I agree to follow this project's Code of Conduct
@remyvv remyvv added the enhancement New feature or request label Oct 31, 2022
@gmazzap
Copy link
Contributor

gmazzap commented Oct 31, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants