Convert update_calculate_project_update_status() into a class

We should get #3074993: Use "current" in update URLs instead of CORE_COMPATIBILITY to retrieve all future updates in first because it is critical

We should also get #3099825: Test available updates when the current major and next major of core are supported in first because it adds more test coverage for what we would be refactoring here

#3074993: Use "current" in update URLs instead of CORE_COMPATIBILITY to retrieve all future updates is RTBC. I would really love to do this issue next before #3100115: The update module should recommend updates based on supported_branches rather than major versions majors which will be a blocker for Drupal.org supporting semantic contrib releases but I don't also want to waste alot time refactoring update_calculate_project_update_status() because #3100115 will again change it.

If we do #3100115 first I fear we are just going to make update_calculate_project_update_status() more of jumbled mess.

So this patch on top of #307499 simply moves update_calculate_project_update_status() into a new class as \Drupal\update\UpdateProjectStatus::getUpdateProjectData().

The logic has not changed at all. My hope would be that we can get this in and then in #3100115 we are free to move our new logic into private functions to make it more understandable.

updating title. update_calculate_project_update_status() is the only function that deals with 1 project at a time

#3074993: Use "current" in update URLs instead of CORE_COMPATIBILITY to retrieve all future updates was committed 🎉

Here is with a couple doc updates.

Notice I didn't update the doc for \Drupal\update\UpdateProjectStatus::getUpdateProjectData() I copied this directly from the current function. There are probably problem with it but the logic of the function didn't change at all. So I don't think we should go down that rabbit hole.

@dww thanks the review. It is amazing that this 10+ year old code has served us well for so long!

re #6

1. removed the private function. Was trying to change the code copied as little as possible and since we don't have the $project_date by reference now without the private function we need to change all the return statement to return it.

   But I agree the private method was weird and just changing the return statements is better.

2. I don't want to change the comment here at all. #3100115: The update module should recommend updates based on supported_branches rather than major versions majors has just become critical and it is now a 9.0.0-beta1 blocker. In that issue we will have to overhaul this methods comment completely so I think discussing that here just delays the critical issue.

   Since the logic is not changing at all and the only code that is changing is the return statement rather relying on the $project_data reference I think fixing the comment is not necessary. Also there are probably a bunch of stuff we could fix about the comment and I would rather not get into that I feel like they should have been handled in #3074993: Use "current" in update URLs instead of CORE_COMPATIBILITY to retrieve all future updates if we missed something we should add follow-up to that. But hopefully I think follow-ups are necessary since this will change anyways in 3100115

3. see above
4. Added all the possible keys that by be updated.
5. Removed the private function

   If we're making this a class just because we like classes, but it turns out that you still need a giant function to do everything we need, I'm less inclined to try to get this into core at all.

   Putting this into a class so we don't have to do that and the update logic in #3100115: The update module should recommend updates based on supported_branches rather than major versions majors. I would like keep 3100115 to only the logic changes needed and not moving it into a class. It is going a lot of changes in that issue and would like to make it as small as we can so we get it right.

   If that's true, let's see a proposed reorganization that actually works and results in something with smaller methods.

   I have started to look at #3100115 and was working on patch on top of this. Here is rough non-work version of this class I was working on https://github.com/tedbow/drupal/blob/3100115-from-3100110-5/core/module...
   note this doesn't have fixes above and probably others but it might give some idea of the new logic that could be split out into new methods

     /**
      * Compares to support branches.
      *
      * Support branches can be in 3 formats, 8.x-1., 1., 1.1.. Comparison should
      * ignore the core prefix and @todo more
      *
      * @param string $branch1
      * @param string $branch2
      *
      * @return int
      *
      */
     private function compareSupportBranches($branch1, $branch2) {
   

   You could have 2.1. or 2. branch this actually after 8.x-1. so the comparison is tricky and benefits from a dedicated docblock

     /**
      * Determines if a support branch would be after a specified version.
      *
      * Support branches can be in 3 formats, 8.x-1., 1., 1.1.. Versions can be
      * in the format 8.x-1.1 or 1.0.1.
      *
      * @param string $supported_branch
      * @param string $version
      */
     private function isSupportBranchAfterVersion($supported_branch, $version) {
   

   again with branches and version in different formats it is tricky

     /**
      * Gets the support branch for a version.
      *
      * @param $version
      *
      * @return string|null
      */
     private function getVersionSupportedBranch($version) {
   

   This is all could maybe wrapped in a new method getTargetSupportBranch() because it is all more complicated then the existing logic for getting the target major. I think this demonstrates why I want have this all i class before #3100115. If the logic was moving files and being split out into these new functions the patch would be much harder to review.

   This is all I have so far and I didn't look into refactoring any of the existing logic that really isn't going to change. That could be non-urgent follow.There is probably the needed for other new methods that I have encountered yet. But I think not adding the new logic directly in the main function is already a clear enough benefit.,

6. fixed
7. fixed the space and changed to ::$projectData so now it is below 80 chars
8. I don't think the memory problem is big enough out weigh benefits I mentioned in 5)
9. I don't think we need unit tests since the logic is not changing also they would all need to be immediately re-written

re #7 I think we will go down the rabbit in #3100115 and that fact that all the logic change will be in 1 file will just make it easier to review.

updated the summary.

the only other thing I wondering is if we should actually more all the methods in update.compare.inc into the class. The new version of \update_calculate_project_data() would be the only method that would need to made public. Nobody should be calling \update_calculate_project_update_status() directly But that would mean \update_calculate_project_update_status() could not call the new version in the and be deprecated. It would have to removed. Maybe we can't do that

+++ b/core/modules/update/src/UpdateProjectStatus.php
@@ -0,0 +1,468 @@
+       $this->projectData;

I had a bunch of these where I meant search/replace return; for return $this->projectData but somehow messed it up.

re #14 yeah I was just addressing test failure didn't notice you had already posted another review

re #11
@dww thanks for the review

1. fixed also update the comment to be more standard for what we use for constructors
2. sure!
3. yeah wasn't going to type hint because they are just copied we are absolutely sure what they are.

   but now see the ultimately come from the xml. Adding type hints and removing the comment about where they are copied from.
   also update fetch_status

4. Nope just forgot to remove the quotes from this one when I figured out they weren't needed.
5. fixed. also add (string) figured out this comes from the xml directly
6. yep see
   $this->projectData['also'][$release_major_version] = $version;
   I think the idea here is you maybe in support major like 8.x-1.x but there multiple new majors like 8.x-2.x and 8.x-3.x
7. already fixed
8. Change say it will be removed in 10.0.0

   I guess we need to make change record unless update_calculate_project_update_status() was never to be called directly.(also see #9)

I talked with @tim.plunkett about this issue too. He had some the concerns that @dww had about just making it a class without actually refactoring but also saw my point about #3100115: The update module should recommend updates based on supported_branches rather than major versions majors being very hard to review if we don't make the new class first.

Really refactoring this into a value object would be bigger task than we probably have time for now. Because then we would need to update a lot more places in the update module that use this project info.

So as an alternative is we could really just make a new utility class that has a public static method that keeps a similar signature as update_calculate_project_update_status() in that takes the project data by reference. So instead of doing
$projects[$project] =(new UpdateProjectStatus($projects['drupal'], $available['drupal']))->getUpdateProjectData()

We would do something like
UpdateProjectCalculator::calculateStatus($projects['drupal'], $available['drupal']);

This would still make it possible change the logic in #3100115: The update module should recommend updates based on supported_branches rather than major versions majors and split the new logic into methods without introducing new global functions(which then become defacto apis) and not adding a bunch of new anonymous functions like we added $is_in_supported_branch that are more difficult to document.

I will work on this.

here is the idea in #16 basically changing a value object to a static helper function.

I haven't had a chance to fix coding standard issues but this should at least show the idea

Fixing coding standards for comments. I didn't fix everything in the original function but since it is indented more I fixed that.

🤷‍♂️👽

+++ b/core/modules/update/update.compare.inc
@@ -141,60 +142,13 @@ function update_calculate_project_data($available) {
+ * \Drupal\update\UpdateProjectStatus::getUpdateProjectData().
+ * @deprecated in Drupal 8.9.0 and will be removed before Drupal 10.0.0. There
+ * is no replacement. To get the calculated project update status for all
+ * projects update_calculate_project_data() should be used.

I suggested update_calculate_project_data() as an alternative because this is the only place the original update_calculate_project_update_status() was called. Before it was called we have

$projects = \Drupal::service('update.manager')->getProjects();
update_process_project_info($projects);

And the logic expects this. So it seems like update_calculate_project_data() should be the entry point for the API.

if a module needed to call it just for 1 module they could do
update_calculate_project_data([$available['my_module']])

They would have until Drupal 10 to fix this.

Created a related issue #3206293: Create ProjectRelease class to represent Update XML releases

I am starting to work on the Automatic Updates functionality and found myself accessing the release data via array keys. So I think that would something nice to do before we have the new module. Also I think it makes the current function update_calculate_project_data more readable.

Pushing up my work in progress on another idea.

+++ b/core/modules/update/update.compare.inc
@@ -111,7 +112,7 @@ function update_calculate_project_data($available) {
-      update_calculate_project_update_status($projects[$project], $available[$project]);
+      UpdateProjectStatus::calculateProjectStatus($projects[$project], $available[$project]);

My previous idea was just swap this function directly for a class but now I think it is not a great to just pass in $projects[$project] by reference and have it add values directly to this array.

So I started a Merge Request from scratch that has a new class called ProjectStatusCalculator that will have well defined methods like getRecommendReleaseForTargetMajor()

So the new merge request will leave update_calculate_project_update_status() in place for now but replace a lot of its logic with calls to the methods on ProjectStatusCalculator

I think the new MR kind of handles points 2) and 3) from #21

I am not opposed to the other points but I am not sure we have to do them all in 1 issue. If we only handled part of it I think we would want to rescope this issue.

from #21

We could iterate over the release history 4 times, in 4 separate/targeted protected helper functions, that each handle one of these jobs. Then we're not mixing up responsibilities, there will be no weird side effects, it'll be easier to document (and test!) each method, and each one will be a lot shorter and less magical than what we have now.

That is very similar to what I am doing in this MR.

the new

/**
   * Gets all the installable releases up to and including the existing version.
   *
   * @return array[]
   *   The releases.
   */
  private function getInstallableReleases(): array {

allows 4 separate new public functions on the class to not have handle the actually looping and filtering of releases that should not be consider in any of the cases, and aren't now.

Also because it keeps static $releases even though each of the helper functions can all it but iterating through all releases will still only happen once.

I have to take off now but will look back more at #21 to see how it compares to this and leave a more detailed comment

Note more tests are passing with the last few commits
PHP 8.0 & MySQL 5.7 6 pass, 8 fail
Only show if the test class all pass not individual test methods.

I thinking has evolved since I first made this issue 😜

re @dww in #7

IMHO, this patch is of almost no use if it just wraps the huge function (and comment) in class syntax but otherwise leaves it as-is.

I mostly agree with this now. It would be of some use but I won't leave us in a much better state. My first plan was we would then iterate over the class in follow-up issue but.....

re @dww in #6

If only Drupal was OO friendly and API-by-array wasn't the Drupal Way(tm) to do everything when I wrote all this in the first place, perhaps you wouldn't be faced with this annoying task now. ;)

I think another problem with my patches in #18 and before I was still making a class that was just passing the $project_data array by reference and altering it so continuing at our API-by-array problem

In the current merge request(1537) I have changed this to follow the pattern we used in #3206293: Create ProjectRelease class to represent Update XML releases. We created the public factory method \Drupal\update\ProjectRelease::createFromArray() and kept the constructor private. This way it allows in the future, if it makes sense, when we are able to not depend on the array as API to deprecate createFromArray() and either create another factory or make the constructor public. Then everywhere we need information about the release we no longer have to rely on the array structure but just call the public methods like ProjectRelease::getDownloadUrl()

We can't do much about the fact that update_calculate_project_update_status() follows the API-by-array pattern. It is public function so we can't just remove it or change it's signature. At some point we should probably deprecate it but I am not sure we can do that before Drupal 10. We can however change it internally to as much as possible to just alter the array by calling public methods on well scoped classes. This what I am trying to do in the merge request. I am not sure if we need to do it all in 1 issue.

Back to @dww suggestions in #21

1. For project status or really $project_data['status'] as this is set in the currently this is bit tricky to replace completely with ProjectStatusCalculator::getStatus() by doing something like

   $project_data['status'] = $status_calculator->getStatus();

   Because for instance
   $project_data['status'] = UpdateManagerInterface::NOT_SUPPORTED
   appears multiple times. Once it is stores project_status from the XML, another time it is because the installed version is not in a supported branch and another time it is because installed version is marked unsupported.

   I don't think we want to cover all those cases in getStatus() of the new class because the method would give no indication of how whether that status reflects project status on Drupal.org from the XML or is related to the specific release they had have installed.
   Currently update_calculate_project_update_status() handles this by in some cases by setting $project_data['extra'] and other cases by settings $project_data['reason'] to give more context. But these seem very tied to how they will be displayed because they seem to both store the human readable "reason" for that status.

   One way to handle this would to not move the "Reason" and "Extra" logic into the new class in this issue. The current merge request adds another class UpdateServerProjectInfo(open to a better name) we could add UpdateServerProjectInfo::getStatus() and let the logic for reacting to that stay in update_calculate_project_update_status() for now.

2. Finds the recommended, and optionally latest/also available releases we want to handle and inform admins about.

   These are covered by the new public methods on ProjectStatusCalculator: getLatestReleaseForTargetMajor(), getRecommendReleaseForTargetMajor(), getReleasesInMajorsGreaterThanTarget()currently "also" getDevReleaseForTargetMajor()and getLatestDev()

3. re security releases, this handled by getSecurityReleases()

Tagging with "Automatic Updates Initiative" because this would make it much use to write a new method like getRecommendReleaseInTargetMinor()