Problem/Motivation

There's a lot of code that has access to the container that uses the \Drupal static class or procedural functions to access stuff that has a service equivalent. This seems like a good step to prepare the codebase for #3165795: PWA module 3.x roadmap.

Steps to reproduce

Apply eyeballs to code.

Proposed resolution

Rewrite everything that has access to the container to inject dependencies wherever possible.

Remaining tasks

I'm going to create a branch and then open a merge request with a lot of the changes since I enjoy this kind of work.

User interface changes

None.

API changes

Does changing the constructor count as API changes?

Data model changes

See previous heading.

Issue fork pwa-3240962

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

Ambient.Impact created an issue. See original summary.

ambient.impact’s picture

ambient.impact
they/them
Primary language English
Location Toronto
commented
Related issues: +#3165795: PWA module 3.x roadmap

Adding related issue #3165795: PWA module 3.x roadmap because I forgot to.

ambient.impact’s picture

ambient.impact
they/them
Primary language English
Location Toronto
commented
Status: Active » Needs review

I've pushed a commit with the bulk of the changes so could use some feedback and testing to make sure I didn't break anything.

Ambient.Impact opened merge request !12

alexborsody’s picture

alexborsody commented

Testing this today. Then need to reroll and merge.

ambient.impact’s picture

ambient.impact
they/them
Primary language English
Location Toronto
commented

I appreciate it. I'm guessing the merge error is in pwa.serv‎ices.yml‎ as both this merge request and another change the arguments key for the pwa.manifest service from a single line array to a multi line one and Git gets confused.

alexborsody’s picture

alexborsody commented
Version: 2.x-dev » 8.x-1.x-dev
ambient.impact’s picture

ambient.impact
they/them
Primary language English
Location Toronto
commented

@AlexBorsody I've rebased my local repository to 8.x-1.x - do you want me to push that to the existing merge request branch or do you want me to create another branch?

Edit: nevermind, created a new branch and opened a second merge request for 8.x-1.x

Ambient.Impact opened merge request !16

alexborsody’s picture

alexborsody commented

Yeah I was thinking we could stay on this branch, reviewing today and want to merge ASAP, looks great.

ambient.impact’s picture

ambient.impact
they/them
Primary language English
Location Toronto
commented

@AlexBorsody Alright, sounds good. Also, I totally forgot to integrate the latest commits to the issue fork's 8.x-1.x branch so I rebased and force pushed. Whoops.

alexborsody’s picture

alexborsody commented
Status: Needs review » Fixed
alexborsody’s picture

alexborsody commented
Status: Fixed » Closed (fixed)
ambient.impact’s picture

ambient.impact
they/them
Primary language English
Location Toronto
commented

Excellent, thanks!

AlexBorsody closed merge request !12

Ambient.Impact credited Megha_kundar.

Ambient.Impact credited ankitsingh0188.

Ambient.Impact credited ankushgautam76@gmail.com.

Ambient.Impact credited mohrerao.

Ambient.Impact credited nguyenphan.

ambient.impact’s picture

ambient.impact
they/them
Primary language English
Location Toronto
commented

Adding credit from #3131591: Drupal 9 deprecated code report

Ambient.Impact credited Eli-T.

Ambient.Impact credited Gábor Hojtsy.

Ambient.Impact credited Kristen Pol.

Ambient.Impact credited codersukanta.

Ambient.Impact credited nod_.

Ambient.Impact credited tyler36.

ambient.impact’s picture

ambient.impact
they/them
Primary language English
Location Toronto
commented