Problem/Motivation

  • update_process_project_info
  • update_calculate_project_data
  • update_calculate_project_update_status

Steps to reproduce

Proposed resolution

Create a new internal UpdateCalculator service

  • public processProjectInfo
  • public updateProjectStatus

Add new method to UpdateManagerInterface

  • public calculateProjectData

Create an UpdateProject value object
Create an UpdateServerProjectInfo value object

Remaining tasks

Review

User interface changes

N/A

Introduced terminology

N/A

API changes

Data model changes

N/A

Release notes snippet

N/A

CommentFileSizeAuthor
#28 3580705-nr-bot_4gqd_r27.txt1.03 KBneeds-review-queue-bot
#25 3580705-nr-bot_z57wqt8j.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3580705

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

nicxvan created an issue. See original summary.

nicxvan’s picture

nicxvan commented
Issue summary: View changes
dww’s picture

dww
we/he/they
commented
Related issues: +#3100110: Convert update_calculate_project_update_status() into a class

Not duplicate, but definitely related to #3100110: Convert update_calculate_project_update_status() into a class. I haven’t had time or urgency to circle back to that issue. I sort of remember not fully agreeing with where it ended up, but there’s a lot of prior work in there that needs to either be refreshed and finished or at least considered before proceeding here.

My initial concern with this plan (and its siblings) is we already have some weird services that are sort of incomplete and interact with each other in “interesting” ways. I wasn't around core for a few years when folks started OOP-ifying this module. I don't love any of how it’s currently split up. I'd love to take some time (which I don't have in the next ~3 weeks) to really think about it and come up with a more wholistic plan for how we should organize all this. I'd rather do that before we start introducing new services.

nicxvan’s picture

nicxvan commented

We will do our best not to make this worse, but we do want to deprecate and move these functions.

nicxvan’s picture

nicxvan commented

Didn't do deprecations for new parameters, not sure how to handle a final and private constructor either.

I also didn't inject key value expirable, still need to do that.

I didn't update the CR yet.

nicxvan opened merge request !15524

nicxvan’s picture

nicxvan commented

For the private final classes I just added the new parameter.
I added the deprecation.

I also added a custom CR for this that needs updating.

nicxvan’s picture

nicxvan commented
Status: Active » Needs review

This is ready for review.

nicxvan’s picture

nicxvan commented
Status: Needs review » Needs work

Strict types is causing a couple of failures, I can't review why right now though.

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review

I reached out to @godotislate in slack for help and here helped be find the cause in the build tests.

It turns out it's actually pretty deep in the extension so not something to fix here.

@godotislate said it would be OK to drop strict types and leave a comment.

Edit: definitely need to give @godotislate credit, but I'm on my phone and not logged into new.d.o

nicxvan’s picture

nicxvan commented
Status: Needs review » Needs work

Got a few things to address, thanks!

nicxvan’s picture

nicxvan commented

Ok I addressed most of the feedback, we still need to introduce the value object for project information.

nicxvan opened merge request !15907

nicxvan’s picture

nicxvan commented

Ok I pushed the first value object, it introduced some failures though to track down.

nicxvan opened merge request !15928

nicxvan’s picture

nicxvan commented

I added the updateproject value object, one of the logic branches changed and I haven't tracked it down yet.

nicxvan’s picture

nicxvan commented
Issue summary: View changes

nicxvan changed the visibility of the branch 3580705-deprecate-update.compare-functions to hidden.

nicxvan changed the visibility of the branch 3580705-update.comparev2 to hidden.

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review

Ok I think this is ready for review, I created two value objects for the main method.

This issue is large, but we only touch 3 functions so hopefully it's ok, we add two value objects to help with the boundary of project comparison. The only logic changes were converting from an array to an object.

I took inspiration from the issue mentioned in 3, but kept the UpdateProject as a strict value object.

This could use further refactoring, but that should be done in follow ups.

nicxvan’s picture

nicxvan commented

Added suggestions for 11.5 deprecation since this almost certainly won't make it into 11.4

nicxvan’s picture

nicxvan commented
Status: Needs review » Needs work

I updated most feedback but some tests broke.

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review
nicxvan’s picture

nicxvan commented

Wrong issue

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3580705-nr-bot_z57wqt8j.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review

Rebased after update.module got in!

nicxvan opened merge request !16929

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3580705-nr-bot_4gqd_r27.txt1.03 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review

I've addressed the latest round of feedback, this should be ready for review again.

nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Issue summary: View changes
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

I think this is pretty OK now.

We've moved stuff around compared to earlier iterations, most importantly, the main method moved to UpdateManager as it also deals with storage and then we can make that internal to UpdateManager and will allow to deprecate some rather strange public methods. This means UpdateCalculator needs to use public methods now, but it also makes the two value objects introduced here make slightly more sense as they're actually used at an API boundary (tagged with internal though) when previously it was just a protected method. The idea is that we could expose them more by returning them from the UpdateManager methods, likely with some kind of ArrayAccess BC layer. We'll see if that actually happens :)

nicxvan opened merge request !16999

  • amateescu committed 4a54dacd on main 
    task: #3580705 Deprecate update.compare functions

By: nicxvan
By:...
amateescu’s picture

amateescu commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed and pushed 4a54dacd42d to main. Thanks!

Waiting for the 11.x MR to turn green.

amateescu’s picture

amateescu commented
Status: Patch (to be ported) » Fixed

Committed bed3ba6 and pushed to 11.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • amateescu committed bed3ba6a on 11.x 
    task: #3580705 Deprecate update.compare functions

By: nicxvan
By:...

amateescu opened merge request !17000

amateescu’s picture

amateescu commented
Status: Fixed » Needs review

Opened a quick followup MR because we forgot to update the change record links.

nicxvan’s picture

nicxvan commented
Status: Needs review » Reviewed & tested by the community

Updated CR looks right! I'll keep an eye on tests.

  • amateescu committed 50100410 on 11.x 
    task: #3580705 followup - Update the change record links

By: nicxvan
By...

  • amateescu committed 2689e346 on main 
    task: #3580705 followup - Update the change record links

By: nicxvan
By...
amateescu’s picture

amateescu commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed the followup 2689e34 to main and 5010041 to 11.x.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.