Skip to content

[#3019] Fixed 'getProtectedValue()' reading the property from the reflection class instead of the object. - #3020

Open
AlexSkrypnyk wants to merge 7 commits into
mainfrom
feature/3019-reflection-getvalue
Open

[#3019] Fixed 'getProtectedValue()' reading the property from the reflection class instead of the object.#3020
AlexSkrypnyk wants to merge 7 commits into
mainfrom
feature/3019-reflection-getvalue

Conversation

@AlexSkrypnyk

@AlexSkrypnyk AlexSkrypnyk commented Aug 15, 2026

Copy link
Copy Markdown
Member

Closes #3019

Summary

ReflectionTrait::getProtectedValue() passed the ReflectionClass built from $object::class to ReflectionProperty::getValue() instead of the object being inspected, so every call against a non-static property raised ReflectionException: Given object is not an instance of the class this property was declared in. Static properties ignore that argument, which is why the defect stayed hidden. The trait ships to every consumer project at web/modules/custom/<module>_base/tests/src/Traits/, and ahoy update-vortex re-renders it, so a consumer's local fix was reverted on the next update.

The method also had no test coverage anywhere in the repository, which is why the defect survived. Rather than shipping a test into every consumer site, the shipped test traits are now covered by a unit suite inside the .vortex/ harness, which never reaches consumer projects.

Changes

Fix

  • web/modules/custom/ys_base/tests/src/Traits/ReflectionTrait.php: getProtectedValue() now calls $property->getValue($object) instead of $property->getValue($class). Its parameters gained object $object / string $property type hints so all three methods on the trait are consistently typed, and the @param $object description was corrected from "Object to set the value on." to "Object to get the value from."

Coverage for the shipped test traits

  • .vortex/tests/phpunit/Unit/ReflectionTraitTest.php: covers getProtectedValue() (instance property across seven value types, reading from the specific instance passed in, static property, inherited property, unknown property), setProtectedValue(), and callProtectedMethod() including its three error paths.
  • .vortex/tests/phpunit/Unit/ArrayTraitTest.php, AssertTraitTest.php, MockTraitTest.php: cover the remaining traits that can run outside a Drupal bootstrap. BrowserHtmlDebugTrait is not covered because it depends on DRUPAL_ROOT and createScreenshot() from Drupal's browser test base.
  • .vortex/tests/composer.json: maps Drupal\Tests\ys_base\Traits\ onto the template directory so the suite exercises the exact files consumers receive, and adds test-unit and test-unit-coverage scripts.
  • .vortex/tests/phpunit.xml: adds a unit testsuite and narrows default to phpunit/Functional so nothing runs twice. Coverage sets disableCodeCoverageIgnore because the traits carry @codeCoverageIgnore to keep them off a consumer site's own coverage figures.
  • .github/workflows/vortex-test-common.yml: runs the unit suite once, on batch 0 of the existing vortex-test-workflow job, writing Cobertura output into the directory already uploaded to Codecov. The runner image ships pcov, so no coverage driver setup is needed; pcov.directory is widened to the repository root because the traits sit outside the working directory.

Current figures: ReflectionTrait, ArrayTrait and AssertTrait at 100% of lines and methods; MockTrait at 86% of lines, since its Stub branch cannot be reached now that PHPUnit 12 has removed TestCase::returnValue().

Fixtures

  • .vortex/installer/tests/Fixtures/handler_process/**: regenerated with ahoy update-snapshots so every scenario's snapshot of the trait matches the fixed source, including the docroot/-hosted Acquia scenarios and the renamed-module scenario.

Before / After

BEFORE - getValue() receives the ReflectionClass

  getProtectedValue($object, $property)
            |
            v
  $class = new ReflectionClass($object::class)
            |
            v
  $property->getValue($class)   <-- expects the instance, gets the reflection
            |
      +-----+-----+
      |           |
      v           v
  instance     static
  property     property
      |           |
      v           v
  ReflectionException:      value returned
  "Given object is not      (argument unused for
  an instance of the        static properties, so
  class this property       the defect stayed
  was declared in"          hidden)


AFTER - getValue() receives the object

  getProtectedValue($object, $property)
            |
            v
  $class = new ReflectionClass($object::class)
            |
            v
  $property->getValue($object)  <-- the instance to read from
            |
      +-----+-----+
      |           |
      v           v
  instance     static
  property     property
      |           |
      v           v
  value returned      value returned
  from $object        (unchanged)


COVERAGE - where the traits are exercised

  web/modules/custom/ys_base/tests/src/Traits/   (ships to consumers)
      ArrayTrait, AssertTrait, MockTrait, ReflectionTrait
                        ^
                        | PSR-4 mapping, no test file ships
                        |
  .vortex/tests/phpunit/Unit/                    (stripped on install)
      ArrayTraitTest, AssertTraitTest, MockTraitTest, ReflectionTraitTest
                        |
                        v
      composer test-unit-coverage  ->  Cobertura  ->  Codecov

@github-project-automation github-project-automation Bot moved this to BACKLOG in Vortex 1.x Aug 15, 2026
@AlexSkrypnyk AlexSkrypnyk added this to the 1.41.0 milestone Aug 15, 2026
@AlexSkrypnyk AlexSkrypnyk added the A2 Working clone index A2 label Aug 15, 2026
@coderabbitai

coderabbitai Bot commented Aug 15, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 68a1fc46-f9ea-4425-868a-2e7010cea129

📥 Commits

Reviewing files that changed from the base of the PR and between 32be59c and e1b7e61.

⛔ Files ignored due to path filters (18)
  • .vortex/installer/tests/Fixtures/handler_process/_baseline/web/modules/custom/sw_base/tests/src/Traits/ReflectionTrait.php is excluded by !.vortex/installer/tests/Fixtures/**
  • .vortex/installer/tests/Fixtures/handler_process/_baseline/web/modules/custom/sw_base/tests/src/Unit/ReflectionTraitTest.php is excluded by !.vortex/installer/tests/Fixtures/**
  • .vortex/installer/tests/Fixtures/handler_process/custom_modules_no_base/web/modules/custom/sw_base/tests/src/Unit/-ReflectionTraitTest.php is excluded by !.vortex/installer/tests/Fixtures/**
  • .vortex/installer/tests/Fixtures/handler_process/custom_modules_none/web/modules/custom/sw_base/tests/src/Unit/-ReflectionTraitTest.php is excluded by !.vortex/installer/tests/Fixtures/**
  • .vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/modules/custom/sw_base/tests/src/Traits/ReflectionTrait.php is excluded by !.vortex/installer/tests/Fixtures/**
  • .vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/modules/custom/sw_base/tests/src/Unit/ReflectionTraitTest.php is excluded by !.vortex/installer/tests/Fixtures/**
  • .vortex/installer/tests/Fixtures/handler_process/hosting_acquia/web/modules/custom/sw_base/tests/src/Unit/-ReflectionTraitTest.php is excluded by !.vortex/installer/tests/Fixtures/**
  • .vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/modules/custom/sw_base/tests/src/Traits/ReflectionTrait.php is excluded by !.vortex/installer/tests/Fixtures/**
  • .vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/modules/custom/sw_base/tests/src/Unit/ReflectionTraitTest.php is excluded by !.vortex/installer/tests/Fixtures/**
  • .vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/web/modules/custom/sw_base/tests/src/Unit/-ReflectionTraitTest.php is excluded by !.vortex/installer/tests/Fixtures/**
  • .vortex/installer/tests/Fixtures/handler_process/names/web/modules/custom/sw_base/tests/src/Unit/-ReflectionTraitTest.php is excluded by !.vortex/installer/tests/Fixtures/**
  • .vortex/installer/tests/Fixtures/handler_process/names/web/modules/custom/the_force_base/tests/src/Traits/ReflectionTrait.php is excluded by !.vortex/installer/tests/Fixtures/**
  • .vortex/installer/tests/Fixtures/handler_process/names/web/modules/custom/the_force_base/tests/src/Unit/ReflectionTraitTest.php is excluded by !.vortex/installer/tests/Fixtures/**
  • .vortex/installer/tests/Fixtures/handler_process/tools_groups_no_be_tests/web/modules/custom/sw_base/tests/src/Unit/-ReflectionTraitTest.php is excluded by !.vortex/installer/tests/Fixtures/**
  • .vortex/installer/tests/Fixtures/handler_process/tools_groups_no_be_tests_circleci/web/modules/custom/sw_base/tests/src/Unit/-ReflectionTraitTest.php is excluded by !.vortex/installer/tests/Fixtures/**
  • .vortex/installer/tests/Fixtures/handler_process/tools_no_phpunit/web/modules/custom/sw_base/tests/src/Unit/-ReflectionTraitTest.php is excluded by !.vortex/installer/tests/Fixtures/**
  • .vortex/installer/tests/Fixtures/handler_process/tools_no_phpunit_circleci/web/modules/custom/sw_base/tests/src/Unit/-ReflectionTraitTest.php is excluded by !.vortex/installer/tests/Fixtures/**
  • .vortex/installer/tests/Fixtures/handler_process/tools_none/web/modules/custom/sw_base/tests/src/Unit/-ReflectionTraitTest.php is excluded by !.vortex/installer/tests/Fixtures/**
📒 Files selected for processing (2)
  • web/modules/custom/ys_base/tests/src/Traits/ReflectionTrait.php
  • web/modules/custom/ys_base/tests/src/Unit/ReflectionTraitTest.php

Included review availability: 0 reviews are currently available. Based on recent review activity, included reviews refill at 1 per hour.


Walkthrough

The reflection helper now reads protected properties from the supplied object. A PHPUnit test suite covers instance, static, inherited, mixed-value, and missing-property cases.

Changes

Reflection access

Layer / File(s) Summary
Correct protected property access and test coverage
web/modules/custom/ys_base/tests/src/Traits/ReflectionTrait.php, web/modules/custom/ys_base/tests/src/Unit/ReflectionTraitTest.php
getProtectedValue now requires an object and property name, then reads the property from that object. Tests cover multiple values, static and inherited properties, instance selection, and missing-property exceptions.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to e1b7e

This PR corrects protected instance-property reads and adds regression coverage plus updated installer fixtures. No actionable merge-blocking risk remains beyond normal checks and review.

Possibly related issues

  • drevops/vortex#3019 — Directly describes and tests the ReflectionTrait::getProtectedValue() defect fixed here.

Poem

A rabbit found a hidden field,
And made its proper value yield.
Instance, static, parent too,
Missing names now signal true.
Hop, hop—tests guard the way!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main reflection fix implemented by the pull request.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/3019-reflection-getvalue

Comment @coderabbitai help to get the list of available commands.

@github-actions

This comment has been minimized.

@AlexSkrypnyk

This comment has been minimized.

2 similar comments
@AlexSkrypnyk

This comment has been minimized.

@AlexSkrypnyk

This comment has been minimized.

@github-actions

github-actions Bot commented Aug 15, 2026

Copy link
Copy Markdown

📖 Documentation preview for this pull request has been deployed to Netlify:

https://6a8103c1ea98cb7affdc3d85--vortex-docs.netlify.app

This preview is rebuilt on every commit and is not the production documentation site.

@github-actions

This comment has been minimized.

@AlexSkrypnyk

This comment has been minimized.

2 similar comments
@AlexSkrypnyk

This comment has been minimized.

@AlexSkrypnyk

This comment has been minimized.

@codecov

codecov Bot commented Aug 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.71%. Comparing base (32be59c) to head (9c4a54a).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3020      +/-   ##
==========================================
- Coverage   87.12%   86.71%   -0.41%     
==========================================
  Files         101       94       -7     
  Lines        4917     4758     -159     
  Branches       47        3      -44     
==========================================
- Hits         4284     4126     -158     
+ Misses        633      632       -1     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

This comment has been minimized.

@AlexSkrypnyk

This comment has been minimized.

2 similar comments
@AlexSkrypnyk

This comment has been minimized.

@AlexSkrypnyk

This comment has been minimized.

@AlexSkrypnyk AlexSkrypnyk changed the title [#3019] Fixed 'getProtectedValue()' passing the reflection class to 'ReflectionProperty::getValue()' instead of the object. [#3019] Fixed 'getProtectedValue()' reading the property from the reflection class instead of the object. Aug 16, 2026
@github-actions

Copy link
Copy Markdown

Code coverage (threshold: 90%)

  Classes: 100.00% (1/1)
  Methods: 100.00% (2/2)
  Lines:   98.56% (206/209)
Per-class coverage
Drupal\ys_demo\Plugin\Block\CounterBlock
  Methods: 100.00% ( 2/ 2)   Lines: 100.00% ( 10/ 10)

@AlexSkrypnyk

This comment has been minimized.

@AlexSkrypnyk

This comment has been minimized.

1 similar comment
@AlexSkrypnyk

Copy link
Copy Markdown
Member Author

Code coverage (threshold: 90%)

  Classes: 100.00% (1/1)
  Methods: 100.00% (2/2)
  Lines:   98.56% (206/209)
Per-class coverage
Drupal\ys_demo\Plugin\Block\CounterBlock
  Methods: 100.00% ( 2/ 2)   Lines: 100.00% ( 10/ 10)

@AlexSkrypnyk AlexSkrypnyk added the Needs review Pull request needs a review from assigned developers label Aug 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A2 Working clone index A2 Needs review Pull request needs a review from assigned developers

Projects

Status: BACKLOG

Development

Successfully merging this pull request may close these issues.

Fix 'ReflectionTrait::getProtectedValue()' passing the ReflectionClass instead of the object

1 participant