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

VACMS-16882: Resolves Error: Call to a member function getValue() on null #16900

Conversation

dsasser
Copy link
Contributor

@dsasser dsasser commented Jan 18, 2024

Description

Resolves a bug in addToNationalOutreachCalendar() which can cause a php error.

Relates to #16882

Testing done

Tested using different user roles, and using new and existing Event nodes.

QA steps

As a content admin

  1. Visit /node/add/event
  2. Fill out the required fields
  3. In the right sidebar, under "Feature This Content", check the "Featured" checkbox
  4. Save the node
    • Validate that there are no errors

Definition of Done

  • Documentation has been updated, if applicable.
  • Tests have been added if necessary.
  • Automated tests have passed.
  • Code Quality Tests have passed.
  • Acceptance Criteria in related issue are met.
  • Manual Code Review Approved.
  • If there are field changes, front end output has been thoroughly checked.

Select Team for PR review

  • CMS Team
  • Public websites
  • Facilities
  • User support
  • Accelerated Publishing

@dsasser dsasser requested review from a team as code owners January 18, 2024 17:21
Copy link

GitHub Workflows (.github/workflows/*.yml)

Have you...

  • pinned all affected GitHub Actions at a specific commit by SHA?
  • reviewed the source code of the action at the commit you are pinning?
  • confirmed that no GitHub security measures are being bypassed?
  • checked for any injection of user content into protected contexts?
  • reviewed Security hardening for GitHub Actions?
  • reviewed GitHub Workflows?

@va-cms-bot va-cms-bot temporarily deployed to Tugboat January 18, 2024 17:21 Destroyed
@va-cms-bot va-cms-bot temporarily deployed to Tugboat January 18, 2024 17:39 Destroyed
@va-cms-bot va-cms-bot temporarily deployed to Tugboat January 18, 2024 17:45 Destroyed
@va-cms-bot va-cms-bot temporarily deployed to Tugboat January 18, 2024 17:59 Destroyed
@va-cms-bot va-cms-bot added the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 18, 2024
@github-actions github-actions bot removed the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 18, 2024
@va-cms-bot va-cms-bot temporarily deployed to Tugboat January 18, 2024 19:16 Destroyed
@ndouglas
Copy link
Contributor

Rebuilding because of Tugboat antics.

@va-cms-bot va-cms-bot added the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 19, 2024
@github-actions github-actions bot removed the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 19, 2024
@va-cms-bot va-cms-bot added the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 19, 2024
@github-actions github-actions bot removed the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 19, 2024
@va-cms-bot va-cms-bot added the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 19, 2024
@github-actions github-actions bot removed the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 19, 2024
@va-cms-bot va-cms-bot temporarily deployed to Tugboat January 19, 2024 17:18 Destroyed
@va-cms-bot va-cms-bot temporarily deployed to Tugboat January 19, 2024 17:33 Destroyed
@va-cms-bot va-cms-bot added the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 19, 2024
@github-actions github-actions bot removed the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 19, 2024
@va-cms-bot
Copy link
Collaborator

va/tests/status-error:


entity_update
Title �[31mEntity/field definitions�[39m
Severity �[31mError�[39m
�[31mValue �[39m�[31mMismatched entity and/or field definitions�[39m�[31m �[39m
�[31m �[39m

…ember-function-on-null-in-addToNationalOutreachCalendar
@va-cms-bot va-cms-bot temporarily deployed to Tugboat January 19, 2024 20:19 Destroyed
Becapa
Becapa previously approved these changes Jan 19, 2024
Copy link
Contributor

@Becapa Becapa left a comment

Choose a reason for hiding this comment

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

Looks good. I was able to save the node:
Screenshot 2024-01-19 at 3 58 03 PM

@github-actions github-actions bot removed the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 20, 2024
@va-cms-bot va-cms-bot added the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 20, 2024
@github-actions github-actions bot removed the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 20, 2024
@va-cms-bot va-cms-bot added the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 20, 2024
@github-actions github-actions bot removed the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 20, 2024
@va-cms-bot va-cms-bot added the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 20, 2024
@github-actions github-actions bot removed the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 20, 2024
@va-cms-bot va-cms-bot added the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 21, 2024
@github-actions github-actions bot removed the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 21, 2024
@va-cms-bot va-cms-bot added the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 21, 2024
@github-actions github-actions bot removed the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 21, 2024
@va-cms-bot va-cms-bot added the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 21, 2024
@github-actions github-actions bot removed the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 21, 2024
@va-cms-bot va-cms-bot added the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 21, 2024
@github-actions github-actions bot removed the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 21, 2024
@va-cms-bot va-cms-bot added the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 22, 2024
@github-actions github-actions bot removed the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 22, 2024
@va-cms-bot va-cms-bot added the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 22, 2024
@github-actions github-actions bot removed the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 22, 2024
@va-cms-bot va-cms-bot added the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 22, 2024
@github-actions github-actions bot removed the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 22, 2024
@va-cms-bot va-cms-bot temporarily deployed to Tugboat January 22, 2024 16:27 Destroyed
@va-cms-bot va-cms-bot added the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 22, 2024
@github-actions github-actions bot removed the refresh-tugboat-cache Used by GHA Tugboat automation label Jan 22, 2024
Copy link
Contributor

@edmund-dunn edmund-dunn left a comment

Choose a reason for hiding this comment

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

Worked as expected. No errors! Nice work!!

Copy link
Contributor

@swirtSJW swirtSJW left a comment

Choose a reason for hiding this comment

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

Just one question from me.

$addToCalValue = $node->get(EventOutreachInterface::PUBLISH_TO_OUTREACH_CAL_FIELD)->first()->getValue();
if (isset($addToCalValue['value'])) {
if ($node->get(EventOutreachInterface::PUBLISH_TO_OUTREACH_CAL_FIELD)->first()) {
$addToCalValue = $node->get(EventOutreachInterface::PUBLISH_TO_OUTREACH_CAL_FIELD)->first()->getValue();
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like a good combination to defend against NULL.

$listings = $node->get(EventOutreachInterface::LISTING_FIELD)->getValue();
$additionalListings = $node->get(EventOutreachInterface::ADDITIONAL_LISTING_FIELD)->getValue();
assert(array_key_exists('value', $addToCalValue));
Copy link
Contributor

Choose a reason for hiding this comment

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

I am pretty sure I am missing the use case/edge case here so apologies for the potentially dumb question here.

Is there an edge case where it would get past line 49 and still not have a 'value' here at line 53? If there is, it might be good to spell out with a comment the situation that would lead to the assertion failing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question Steve. My thinking in this case was that there is actually no guarantee that, even though the field has a value, that the property 'value' exists in the returned array. For entity reference fields for example, the primary field property is 'target_id'. So this assertion was a way for me to protect against any future changes to the target field where it changes to a different type, or for some reason the 'value' property changes to something else.

Generally I've been trying to get away from using magic methods to get to field values, which would actually improve the readability of this code, but this is the outcome of doing that in this situation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. Thanks for the explanation @dsasser.

@dsasser dsasser merged commit 2ca4409 into main Jan 22, 2024
22 checks passed
@dsasser dsasser deleted the VACMS-16882-call-to-member-function-on-null-in-addToNationalOutreachCalendar branch January 22, 2024 20:19
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.

7 participants