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

Add magic get/setters for entityreferences (#948) #956

Open
wants to merge 14 commits into
base: 3.x
Choose a base branch
from

Conversation

boobaa
Copy link
Contributor

@boobaa boobaa commented Oct 10, 2023

Closes #948.

@divya-intelli
Copy link
Collaborator

Thank you @boobaa for your contribution.
We will review the PR and update.

Copy link
Contributor

@mxr576 mxr576 left a comment

Choose a reason for hiding this comment

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

Awesome, we are one step closer.

These are my first impressions, I'd not call it a review.

This PR definitely needs extensive manual testing for a start and examining what is actually stored in an object and how these methods behave, e.g.: whether attributes are supported or not by this change, etc.

After this looks like a working solution, automated test coverage would be also a must (IMO).

src/Entity/FieldableEdgeEntityBase.php Show resolved Hide resolved
src/Entity/FieldableEdgeEntityBase.php Show resolved Hide resolved
@boobaa
Copy link
Contributor Author

boobaa commented Oct 11, 2023

Also had to distinguish apigee baseFields: without doing that, /teams page was just a WSOD.

@kedarkhaire
Copy link
Collaborator

Hi @boobaa
I have checked the commits and files on the new installation.
Before implementing the fix, as per @mxr576 steps to reproduce, the issue is WSOD
Post applying the changes, we still see the WSOD following the same steps.

We cannot merge this as it does not solves the mentioned issue. It will need more further work and testing.

Thanks!

@kedarkhaire
Copy link
Collaborator

Hi @boobaa
We have performed the following tests to check the fix post appending your changes.

  1. Created a new API reference field in Teams creation form.
  2. Checked the create new entity if not avaiable option in the field settings page.
  3. Created a Team with new text in the entity reference field and submitted the team.
  4. WSOD was observed.

This led to the failure of manual test from my end.
If there is any specific steps you tried to check the working state of code which gave you positive results please share with us, we will surely look into it.

Thanks!

Copy link

codecov bot commented Dec 22, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (dc96b7d) 44.20% compared to head (b67f189) 44.20%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##                3.x     #956   +/-   ##
=========================================
  Coverage     44.20%   44.20%           
  Complexity     3029     3029           
=========================================
  Files           341      341           
  Lines         11086    11086           
=========================================
  Hits           4901     4901           
  Misses         6185     6185           
Files Coverage Δ
src/Entity/FieldableEdgeEntityBase.php 76.58% <ø> (ø)

@boobaa
Copy link
Contributor Author

boobaa commented Jan 10, 2024

Here's what I did for reproduction:

  1. Installed drupal-10.2.1 from scratch, with minimal profile + field_ui module.
  2. Installed drupal/apigee_edge-3.0.5 and configured it to use an organization on our OPDK server. Connection was successful.
  3. Navigated to /admin/config/apigee-edge/team-settings/fields and created a new Reference field for an API (product).
  4. Navigated to /admin/structure/block and placed the "Team switcher" block.
  5. Navigated to an already-existing team using this "Team switcher" block.
  6. Clicked Edit, typed an already-existing API (product) name to the just added field.
  7. Clicked "Save team".

Expected result: team getting saved properly, with the new field being displayed with the selected API (product).

Actual result without the patch: WSOD (The website encountered an unexpected error. Try again later.), with this in the logs:

Error: Call to a member function getValue() on null in Drupal\Core\Entity\Plugin\Validation\Constraint\ValidReferenceConstraintValidator->validate() (line 128 of /mnt/files/local_mount/build/web/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ValidReferenceConstraintValidator.php).

Actual result with the patch: team got saved properly, with the new field displaying the selected API (product).

FTR, I ran composer require cweagans/composer-patches and added this to the extra section of my root composer.json to apply the patch:

        "patches": {
            "drupal/apigee_edge": {
                "Magic getters/setters support for fieldable Apigee entities - https://github.com/apigee/apigee-edge-drupal/pull/956": "https://patch-diff.githubusercontent.com/raw/apigee/apigee-edge-drupal/pull/956.patch"
            }
        }

Hope this helps.

@kedarkhaire
Copy link
Collaborator

Hi @boobaa

Drupal version - 10.1.8 & 10.2.2
Apigee Edge 3.0.5
checked with Devportal latest version as well as on the Vanilla Drupal 10.2.2 version.
As per the steps you shared we checked and just wanted to clear few points.

  • On point 3, when you are creating a new Reference field for an API, are you selecting this checkbox - Create referenced entities if they don't already exist
  • If yes, then on point 6, are you trying to create a new value in API reference field ?

The steps which you mentioned, in that case, we do not see any error/WSOD on the page without applying any patch.

Issue occurs when the above 2 points are performed.
Just to be on the same page, can you tell us, if the code was checked with these 2 points ?

Thanks!

@boobaa
Copy link
Contributor Author

boobaa commented Jan 19, 2024

Hi @kedarkhaire ,

as the customer does not want to litter their Apigee Edge with random API products when teams are getting created/edited, but only allow users to select from already-existing API products, those options are NOT enabled.

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.

Magic getters/setters support for fieldable Apigee entities
5 participants