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

Introduce "generic" hook and refactor statuses and statuses actions hooks #52

Merged
merged 8 commits into from
Sep 3, 2024

Conversation

gmazzap
Copy link
Contributor

@gmazzap gmazzap commented Aug 29, 2024

The new hook

The main scope for this PT was solving the problem in #47, that is, havign a "generic" hook that can be used to extend the package without previously holding an instance of the hook.

The reason for this are explained in the comments to #47

To quickly recap: when we want to perform something for all the packages of a given group, e.g. all project-specific plugin/themes/libraries without this hook the only way is to:

  • make sure each package has a funcion that returns the package instance
  • have a list of those functions (which has to be maintained manually)
  • call function_exists befoe callign the function

This is hard to maintain and not particularly fast.

Having this hook, we can avoid all the 3 points above.

Statuses and actions refactoring

When discussing the change above in #47, we found out that we had a pretty inconsistent way of dealing with actions hooks triggered because of status changes.

Moreover, we realized that the various status we had were not very consistent either with all the phases. For this reason we agreed to:

  • Extend the list of statuses
  • Have a "map" between statuses and actions and a method to handle action triggering

Additional improvements

PackageProxyContainer

Thanks to the new statuses in this PR and to the new methods introced in #51 it was possible to improve the usage of PackageProxyContainer, basically avoiding its usage when the container is available.ù

Packages connection

The improvend in this and previous PRs already improced package conections, making it more flexible and reliable.

In this PR another improvement is made. When a connection failed, an excption was eagerly thrown.

Take te scenario when a package:

  1. at 'plugins_loaded' connects to another package
  2. at 'wp_footer' uses services from the conntent packages to do something

Currently, if connection fails an exception would be thrown at step 1. But i the request is, for example, redirected or anyway terminates before 'wp_footer', there was no reason to throw and we could have avoided a runtime problem.

This is why now a failed connection emits a failure action (like before) and does not throw. If and when packages form teh connected package wll be accessed, the exeption will happen because services are missing having the connection failed. This way we delay failure as late as possible, but still is is possible to debug the reason for failure (e.g. my logging the "connection failed" action hook).

Tests

A good number of unit tests are added to prove the various possible flows with combinations of build(), boot(), and connect().

Documentation

I took the chance to also add extensive inline documentation to explain the different parts of the bootstrapping flow, especially in the Package class, now it should be clear reading the code why anything does what it does.

The user documentation has also been extended for the new funcionalities and also reworked to be in general more clear.

Notes

If merged, this PR not only closes #47 but also #45. The latter was attempting to solve "inconsistencies" in build and boot methods. Those inconsitencies where caused by two reasons: limitations in PackageProxyContainer which handled here and partly in #49 and #51, and the lack of aligned when build() was introduced, and that is fixed partly here and partly in #49.

In substance, merging this PR and the previous 2, all problems highlithed in #45 are also fixed. In fact, this PR contains a few tests that are very similar to the test proposed in #45


Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes/features)
  • Docs have been added/updated (for bug fixes/features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Feature.

What is the current behavior? (You can also link to an open issue here)

See description.

What is the new behavior (if this is a feature change)?

See description.

Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

No.

Other information:

N/A.

See #47

- Added the generic, statically-named `ACTION_MODULARITY_INIT` action hook
- Refactor hooks triggering, using a map between statuses ans actions, introducing two new statuses: `STATUS_INIT` and `SATUS_DONE`(which replaces the now deprecated `STATUS_BOOTED`)
- Deprecate `STATUS_MODULES_ADDED` (`STATUS_BOOTING` was already an alias)
- Refactor hook triggering for failed connection, moving it to a separate method. Behavior change: connecting an alreayd connected package still fires a failed action hook but does not throw anymore.
- Do not use `PackageProxyContainer` when the package to connect is initialized, considering its container is already available
- Allow for multiple consecutive calls to `Package::boot()` and `Package::build()` avoiding throwing unnecessary exceptions
- Add extensive inline documentation to explain the different parts of the bootstrapping flow
- Add a large amount of tests to cover both existing but untested scenarios as well as the new/updated behaviors
- Rework the "Package" and "Application flow" documentation chapters to make them more easily consumable, better account for latest additions (not limited to the changes in this commit).
@gmazzap gmazzap requested review from tfrommen and Chrico August 29, 2024 11:44
Copy link

codecov bot commented Aug 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.80%. Comparing base (8663163) to head (ab5c8a3).
Report is 1 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master      #52      +/-   ##
============================================
+ Coverage     98.43%   98.80%   +0.36%     
- Complexity      241      251      +10     
============================================
  Files            10       10              
  Lines           576      586      +10     
============================================
+ Hits            567      579      +12     
+ Misses            9        7       -2     
Flag Coverage Δ
unittests 98.80% <100.00%> (+0.36%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/Package.php Outdated Show resolved Hide resolved
Copy link
Member

@tfrommen tfrommen left a comment

Choose a reason for hiding this comment

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

Thanks for this massive effort, espcially on the testing and documentation fronts. 🙏

Personally, I am wondering if this PR should target a new major version, which would allow us to actually get the status constant names and their relationships right, and we could get rid of a bit of deprecated stuff, also around supporting modules passed to boot. It doesn't have to be a breaking change, but I'd like to float the idea...

docs/Application-flow.md Outdated Show resolved Hide resolved
docs/Application-flow.md Outdated Show resolved Hide resolved
docs/Application-flow.md Outdated Show resolved Hide resolved
src/Package.php Outdated Show resolved Hide resolved
src/Package.php Show resolved Hide resolved
src/Package.php Outdated Show resolved Hide resolved
gmazzap and others added 3 commits August 29, 2024 16:01
Co-authored-by: Thorsten Frommen <info@tfrommen.de>
Signed-off-by: Giuseppe Mazzapica <giuseppe.mazzapica@gmail.com>
Co-authored-by: Thorsten Frommen <info@tfrommen.de>
Signed-off-by: Giuseppe Mazzapica <giuseppe.mazzapica@gmail.com>
@gmazzap
Copy link
Contributor Author

gmazzap commented Aug 29, 2024

@tfrommen Having a major version without breaking changes does not make sense to me. We only reduce the reach the improvement can have for no real reason.

Delay these changes for a major release I think it is also not ideal. Having deprecation notices now, will help the transition to a new major when we can break backward compatibility "and get things right".

The main point of the PR, that is the possibility to have a global hook is something that many projects are waiting for, and I think it is good to give them this possibility as soon as possible.

src/Package.php Outdated Show resolved Hide resolved
Copy link
Member

@tfrommen tfrommen left a comment

Choose a reason for hiding this comment

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

Thanks again!

@gmazzap gmazzap merged commit 2119d0e into master Sep 3, 2024
64 checks passed
@gmazzap gmazzap deleted the feature/improve-package-hooks branch September 3, 2024 10:42
@gmazzap gmazzap mentioned this pull request Sep 3, 2024
3 tasks
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.

[Feature Request]: Fire an event when building with a generic hook name to be able to target it easily
3 participants