Add Config and Migration plus review changes #13

aphraoh · 2024-04-02T12:42:25Z

Q	A
Is bugfix?	❌
New feature?	✔️
Breaks BC?	✔️

Co-authored-by: Dmitriy Derepko <[email protected]>

codecov · 2024-04-02T12:53:38Z

Codecov Report

Attention: Patch coverage is 0% with 18 lines in your changes are missing coverage. Please review.

Project coverage is 0.00%. Comparing base (0a408ed) to head (c2b137c).

Files	Patch %	Lines
src/Adapter.php	0.00%	18 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             master     #13   +/-   ##
========================================
  Coverage      0.00%   0.00%           
- Complexity       22      26    +4     
========================================
  Files             1       1           
  Lines            84      95   +11     
========================================
- Misses           84      95   +11

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

Build check error if not added to required

aphraoh · 2024-04-02T13:26:31Z

Hi @vjik and @xepozz us requested except...

@xepozz Did not put "yiisoft/db-migration" to "suggested" section in composer.json as breaks build, please can we deal with this in another pull request.
@vjik not remove the empty di as this also cause problems (can not exactly remember what they were), please can we deal with this in another pull request.

config/di.php

src/Adapter.php

samdark · 2024-04-02T21:11:46Z

src/Adapter.php

@@ -92,7 +97,7 @@ public function push(MessageInterface $message): MessageInterface

    public function subscribe(callable $handlerCallback): void
    {
-        $this->runExisting($handlerCallback);
+        $this->run($handlerCallback, true, 5); // TWK TODO timeout should not be hard coded


Do you plan to resolve it before merge?

Can I do it after the merge, I believe the solution should be somewhere in the middleware but need to investigate a bit further so maybe that can be a lession for another day after the merge...

samdark · 2024-04-02T21:13:31Z

src/Adapter.php

-        // TWK TODO ??? if (!$this->mutex->acquire(__CLASS__ . $this->channel, $this->mutexTimeout)) {
-        // TWK TODO ??? throw new \Exception('Has not waited the lock.');
-        // TWK TODO ??? }
+        // TWK TODO what is useMaster in Yii3 return $this->db->useMaster(function () {


@Tigrov, @terabytesoftw?

samdark · 2024-04-02T21:14:34Z

src/Adapter.php

+    { 
+        while ($this->loop->canContinue()) {
+            if ($payload = $this->reserve()) {
+                if ($handlerCallback(\unserialize($payload['job']))) {


In general, we're not using PHP serialization for queue. Is it necessary?

xepozz · 2024-04-03T06:34:04Z

src/Adapter.php

+    { 
+        while ($this->loop->canContinue()) {
+            if ($payload = $this->reserve()) {
+                if ($handlerCallback(\unserialize($payload['job']))) {


Use https://github.com/yiisoft/queue/blob/master/src/Message/MessageSerializerInterface.php instead

OK, will use this interface

xepozz · 2024-04-03T06:34:10Z

@aphraoh Do not remove discussions, do not use force-push, just add commits one by one, do not care about the rest.

As I wrote before, I don't like yiisoft/db-migration and yiisoft/mutex in require section. If the second is used in the adapter, then first is just declared one files with sql and it maybe useless.

Move it to suggests composer section.

samdark · 2024-04-03T07:29:12Z

just add commits one by one, do not care about the rest.

Yes. We take care about squashing commits when merging, but we need to see how it progressed while doing review. Also, force-pushing is quite confusing for GitHub UI.

Start description with capital letter and end with dot. Co-authored-by: Alexander Makarov <[email protected]>

aphraoh · 2024-04-03T14:45:41Z

just add commits one by one, do not care about the rest.

Yes. We take care about squashing commits when merging, but we need to see how it progressed while doing review. Also, force-pushing is quite confusing for GitHub UI.

Oh dear, please send me a link to the process I should following if I would like to ammend a pull request in github :-|

Update var comments to convention Use serialize interface instead of php serialise

aphraoh · 2024-04-03T15:43:44Z

Hi @xepozz @samdark @vjik , hopefully we can merge with the following outstanding...

// TWK TODO timeout should not be hard coded
// TWK TODO what is useMaster in Yii3
@xepozz, have removed yiisoft/db-migration from composer.json, but yiisoft/mutex still in required section cause used in Adopter (see constructor)

aphraoh · 2024-04-03T15:45:03Z

Apologies in advance if I have done any squeezed and forced commits, still googling to see exactly what you mean by that...

samdark · 2024-04-03T16:31:38Z

@aphraoh latest commits are fine.

samdark · 2024-04-03T16:32:52Z

Let's merge it and address comments separately. Good job, @aphraoh.

aphraoh · 2024-04-04T12:39:36Z

Sent from my iPhoneOn 3 Apr 2024, at 17:33, Alexander Makarov ***@***.***> wrote: Let's merge it and address comments separately. Good job, @aphraoh. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

aphraoh and others added 10 commits March 27, 2024 23:19

Add Config and Migration, Update Adapter.php

b7e4a2d

Update composer.json

b5fe7aa

Update Adapter.php

dd25305

Update Adapter.php

c30e377

Update M240327150317Queue.php

3655908

Update composer.json after review

63bdc76

Use LoopInterface

7fd3788

Use MutexInterface and MutexFactoryInterface

219b3ea

Update src/Adapter.php

2757ef1

Co-authored-by: Dmitriy Derepko <[email protected]>

Update composer.json

98d1613

aphraoh closed this Apr 2, 2024

aphraoh reopened this Apr 2, 2024

Update composer.json

5c917f1

aphraoh closed this Apr 2, 2024

aphraoh reopened this Apr 2, 2024

Update Adapter.php

dfffc04

aphraoh closed this Apr 2, 2024

aphraoh reopened this Apr 2, 2024

aphraoh added 2 commits April 2, 2024 14:15

Update composer.json

7e05e5f

Build check error if not added to required

Update composer.json due to invalid json

6c8f0dd

aphraoh closed this Apr 2, 2024

aphraoh reopened this Apr 2, 2024

samdark reviewed Apr 2, 2024

View reviewed changes

xepozz reviewed Apr 3, 2024

View reviewed changes

aphraoh and others added 2 commits April 3, 2024 15:19

Delete config directory due to empty di

2d04ff1

Update src/Adapter.php

70a3798

Start description with capital letter and end with dot. Co-authored-by: Alexander Makarov <[email protected]>

aphraoh added 3 commits April 3, 2024 15:56

Delete src/Migrations directory

992d64c

Update composer.json

1a9e228

Update Adapter.php

c2b137c

Update var comments to convention Use serialize interface instead of php serialise

aphraoh closed this Apr 3, 2024

aphraoh reopened this Apr 3, 2024

samdark approved these changes Apr 3, 2024

View reviewed changes

samdark merged commit 707c126 into yiisoft:master Apr 3, 2024
26 of 30 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Config and Migration plus review changes #13

Add Config and Migration plus review changes #13

aphraoh commented Apr 2, 2024

codecov bot commented Apr 2, 2024 •

edited

Loading

aphraoh commented Apr 2, 2024

samdark Apr 2, 2024

aphraoh Apr 3, 2024

samdark Apr 2, 2024

samdark Apr 2, 2024

xepozz Apr 3, 2024

aphraoh Apr 3, 2024

xepozz commented Apr 3, 2024

samdark commented Apr 3, 2024

aphraoh commented Apr 3, 2024

aphraoh commented Apr 3, 2024

aphraoh commented Apr 3, 2024

samdark commented Apr 3, 2024

samdark commented Apr 3, 2024

aphraoh commented Apr 4, 2024 via email

Add Config and Migration plus review changes #13

Add Config and Migration plus review changes #13

Conversation

aphraoh commented Apr 2, 2024

codecov bot commented Apr 2, 2024 • edited Loading

Codecov Report

aphraoh commented Apr 2, 2024

samdark Apr 2, 2024

Choose a reason for hiding this comment

aphraoh Apr 3, 2024

Choose a reason for hiding this comment

samdark Apr 2, 2024

Choose a reason for hiding this comment

samdark Apr 2, 2024

Choose a reason for hiding this comment

xepozz Apr 3, 2024

Choose a reason for hiding this comment

aphraoh Apr 3, 2024

Choose a reason for hiding this comment

xepozz commented Apr 3, 2024

samdark commented Apr 3, 2024

aphraoh commented Apr 3, 2024

aphraoh commented Apr 3, 2024

aphraoh commented Apr 3, 2024

samdark commented Apr 3, 2024

samdark commented Apr 3, 2024

aphraoh commented Apr 4, 2024 via email

codecov bot commented Apr 2, 2024 •

edited

Loading