Better Package's Status Checks

[gmazzap]

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)

The only public method to check package status is Package::statusIs(), which only chechs equality. A more flexible status check is possible via Package::checkStatus(), but that is proivate.

Moreover, there's no way to check if the Package has a container already generated. The Package has a hasContainer property, but that is provate.

Because of the two above points, PackageProxyContainer can not 100% determine when it is "safe" to access the proxied container.

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

Three new methods are added:

• Package::isFailed() which is a simpler way to do Package::statusIs(Package::STATUS_FAILED)
• Package::hasReachedStatus() which can check if the status is the one above or above
• Package::hasContainer() which tells when a Container is already created regardless teh status

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

No.

Other information:

This PR includes (in a separate commit) some CS fixes caused by new rules added to the Inpsyde Coding Standards.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.43%. Comparing base (d4ea195) to head (657ed77).
Report is 1 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master      #51      +/-   ##
============================================
+ Coverage     98.42%   98.43%   +0.01%     
- Complexity      238      241       +3     
============================================
  Files            10       10              
  Lines           570      576       +6     
============================================
+ Hits            561      567       +6     
  Misses            9        9              

Flag	Coverage Δ
unittests	98.43% <100.00%> (+0.01%)	⬆️

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.

[tfrommen]

I left a few rather specific inline comments, but here's also some general feedback...

Currently, the status values are integers, starting from 2 and ranging to 8. But we also have a -8, which is a special failed status. And then STATUS_BOOTING is an alias of STATUS_MODULES_ADDED. But also, there are is no status value 3 or 6. If those are just random values that no one really cares about, and you, as a developer, just use the symbols (i.e., you reference the class constants, which are part of the public API), that is fine.

However, now with the introduction of the new hasReachedStatus method, the status values are no longer just "random". The method's internal implementation logic signals that the status values are actually following some form of hierarchy or sequence. If a package's status is 5 (i.e., booting), it seems the package also went through the idle and initialized status before. Which makes sense, from a semantical point of view.

But what would happen if we need to introduce a status that is optional? Or a status that is being reached in between two already existing status values that are consecutive integers, for example, a new status in between initialized and booting. Would we add a float all of a sudden, so that we can still use the simple >= check? Would we change some or all status values? That would be a massive breaking change, of course. Or would we have to add some more complex logic to infer if a package went through some specific status, based on its current status?

Also, hasReachedStatus( STATUS_FAILED /* == -8 */ ) would, by default, be true for any other status, just because they all have larger values. So you added a specific check for the failed status, OK. What would happen if, for some reason, we need to add another failed or special status? Most probably, we would have to update the hasReachedStatus method again, and some or all tests that cover it. What would happen if we had to introduce a new non-failed status that somehow exists parallel to another one? So you either move to an existing status, or that new one, but either of them may or may not have a successor status.

I totally get that this all is somewhat made up, but then again, all is possible and not too far-fetched. So what I am essentially wondering is if we really want to start adding (or making it more explicit) meaning and relationships and sequence between the current status values. Maybe we could add a super small state machine or at least a state map (which could or maybe even should be private). Or maybe we could add an internal list of status values to the package, that is, every time the package status changes, it gets added to the list.

Happy to discuss this here, or elsewhere. Also happy for you to discard this altogether, in case you don't expect these issues to happen anytime soon.

[gmazzap]

Happy to discuss this here, or elsewhere. Also happy for you to discard this altogether, in case you don't expect these issues to happen anytime soon.

Let me recap, to see if I understand your points @tfrommen

I see you basically have 2 points:

1. Introducing hasReachedStatus() method we make the status constants value relevant, because we use it for comparison, when before it was unrelevant.
2. The implementation of the method is poor because, for exaple, might return true passing an argument like 3 when that number points to no status.

---

Let me start from the second point, which is the easiest.

I wish we could use enums here but we can't right now. Introduce a custom class to represent status could work, but it is a pretty big breaking change. So I think it is better to stick with integers. But I totally agree the implementation can be improved, and I will.

---

Regarding the first point, let me start saying that having a linear step-after-step forwarding of statuses (with the exceptions of "failed" which could happen at any time) was a fundational idea on how the status would work. It is even documented this way: https://github.com/inpsyde/modularity/blob/master/docs/Application-flow.md#building-stage

So I don't think in the foresable future this will change. I do think that the actual statuses will change (as we have already dicussed in another issue), but the idea of consequential and enumerable steps is something that I don't see being challenged in the foresable future.

That said, not all the future is foresable :) And this is why I did not just make Package::checkStatus() public. That method exposes the logic of numerical comparison by accepting a comparison operator, and while making it public would have allowed for greater flexibility now, and would have saved me to introduce a new method, it would have forced us to stick with this logic.

By introducing the hasReachedStatus() method, it is true that the statuses constant' value is now relevant, but it is also true that the "comparison logic" is still encapsulated, in other words, the value is relevant internally, not for consumers.

So, if in the non-foreseable future we will introduce optional statuses, multiple error statuses, or anyway something that does not fit the "numerical sequence" logic we have right now, we can still do that without bothering consumers.

Let's assume we introduce a simple state machine, and we use a Statuses class to represent it, we could have someting like Statuses->push($newStatus) whenever the package enters that state, and then we could have the hasReachedStatus() method look like:

    public function hasReachedStatus(int $status): bool
    {
        $this->states->has($status);
    }

For consumers nothing would change. Because we don't expose the comparison logic (even if we use it internally), we can change course if we have to, without breaking backward compatibility.

So, why doing now a more impactful change when, as of today, we don't have any reason to think it will be needed, and if in the future it will be needed we have ways to achieve it without causing much trouble?
Even because, if we will have reasons to have a state machine in the future, maybe in that future we will have PHP 8.1+ as min PHP version, and we would be able to use enums, for example, so a state machine we would write today would be refactored again.

[tfrommen]

Thanks for following up on this!

I suggested a small improvement, but this is good to go either way.