Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Attach behavior via "as behaviorName" with Closure #20248

Merged

Conversation

timkelty
Copy link
Contributor

@timkelty timkelty commented Aug 22, 2024

Allows behaviors attached with the 'as behaviorName' syntax to use a Closure.

In the process of writing a test, I discovered the \yiiunit\framework\base\ComponentTest::testAttachBehavior test was broken (it has assertions that were never reached), so I fixed that as well.

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

Copy link

codecov bot commented Aug 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.95%. Comparing base (eb304e7) to head (8724ba3).
Report is 7 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master   #20248      +/-   ##
============================================
- Coverage     64.95%   64.95%   -0.01%     
- Complexity    11395    11396       +1     
============================================
  Files           430      430              
  Lines         36923    36925       +2     
============================================
- Hits          23985    23984       -1     
- Misses        12938    12941       +3     

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

$this->assertSame($behavior, $component->detachBehavior('a'));
$this->assertFalse($component->hasProperty('p'));
$this->expectException('yii\base\UnknownMethodException');
$component->test();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test was passing but the assertions following this were never run.

@@ -376,6 +380,9 @@ public function testDetachBehavior()

$detachedBehavior = $component->detachBehavior('z');
$this->assertNull($detachedBehavior);

$this->expectException('yii\base\UnknownMethodException');
$component->test();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This made more sense to test here than in attachBehaviors where it lived and was causing the test to end prematurely.

@samdark samdark requested review from a team August 22, 2024 20:10
Copy link
Contributor

@schmunk42 schmunk42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added lines #L193 - L194 were not covered by tests

Could you add a test for that.

@timkelty
Copy link
Contributor Author

Added lines #L193 - L194 were not covered by tests

Could you add a test for that.

@schmunk42 the test for that is here. I'm not sure why that warning is showing in the Github diff. It isn't showing here: https://app.codecov.io/gh/yiisoft/yii2/pull/20248

Copy link
Member

@machour machour left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you rollback changes to the two files under db/ ?

This reverts commit ff60985.
@timkelty
Copy link
Contributor Author

timkelty commented Aug 25, 2024

Can you rollback changes to the two files under db/ ?

@machour Done. CS/lint check will fail, but not from changes in this PR.

@mtangoo
Copy link
Contributor

mtangoo commented Sep 21, 2024

@timkelty thanks for your time and efforts. Can you please resolve the conflict to enable final review and possibly merging it?

# Conflicts:
#	framework/base/Component.php
#	tests/framework/base/ComponentTest.php
@timkelty
Copy link
Contributor Author

All set, @mtangoo.

@mtangoo
Copy link
Contributor

mtangoo commented Sep 23, 2024

All set, @mtangoo.

Thanks for your time and efforts. We will review and merge it if all is good without delay!

@mtangoo mtangoo requested review from machour, schmunk42 and a team September 23, 2024 15:04
@mtangoo
Copy link
Contributor

mtangoo commented Sep 25, 2024

@timkelty the PR is good. Can you add a line in changelog so that we can merge this?

@timkelty
Copy link
Contributor Author

@timkelty the PR is good. Can you add a line in changelog so that we can merge this?

Done!

@mtangoo mtangoo merged commit 3c75ff1 into yiisoft:master Sep 26, 2024
85 of 87 checks passed
@mtangoo
Copy link
Contributor

mtangoo commented Sep 26, 2024

@timkelty thanks for your time and efforts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants