Skip to content

fix conversion of DateTime to string in getContainsHtml#1423

Merged
osma merged 2 commits intomasterfrom
fix-datetime-to-string-in-containshtml
Mar 22, 2023
Merged

fix conversion of DateTime to string in getContainsHtml#1423
osma merged 2 commits intomasterfrom
fix-datetime-to-string-in-containshtml

Conversation

@osma
Copy link
Member

@osma osma commented Mar 22, 2023 •
edited
Loading

Reasons for creating this PR

There is a recently surfaced (probably PHP 8.x related) problem in the YSO-aika (YSO Time) vocabulary. Concept pages failed to display and instead we got this message in the error log:

[Wed Mar 22 09:53:30.124150 2023] [php:error] [pid 709825] [client 127.0.0.1:38096] PHP Fatal error:  Uncaught TypeError: preg_match(): Argument #2 ($subject) must be of type string, DateTime given in /var/www/html/Skosmos/model/ConceptPropertyValueLiteral.php:60

The reason is pretty simple: if a concept has a property with a datetime value, it cannot be implicitly casted to a string value. This PR fixes the problem by relying on the getLabel() method in the same class, which already handles DateTime to string conversion.

Link to relevant issue(s), if any

(not reported as an issue, I decided to fix this directly)

Description of the changes in this PR

  • fix DateTime to string conversion in ConceptPropertyValueLiteral.getContainsHtml() by relying on .getLabel()
  • add unit test for verifying above conversion

Known problems or uncertainties in this PR

none

Checklist

  • phpUnit tests pass locally with my changes
  • I have added tests that show that the new code works, or tests are not relevant for this PR (e.g. only HTML/CSS changes)
  • The PR doesn't reduce accessibility of the front-end code (e.g. tab focus, scaling to different resolutions, use of .sr-only class, color contrast)
  • The PR doesn't introduce unintended code changes (e.g. empty lines or useless reindentation)

@osma osma added the bug label Mar 22, 2023
@osma osma added this to the 2.17 milestone Mar 22, 2023
@osma osma self-assigned this Mar 22, 2023
@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 22, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@codecov
Copy link

codecov bot commented Mar 22, 2023 •
edited
Loading

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (a59f251) 69.55% compared to head (bc490ba) 69.55%.

Additional details and impacted files 
@@            Coverage Diff            @@
##             master    #1423   +/-   ##
=========================================
  Coverage     69.55%   69.55%           
  Complexity     1650     1650           
=========================================
  Files            32       32           
  Lines          4257     4257           
=========================================
  Hits           2961     2961           
  Misses         1296     1296
Impacted Files Coverage Δ
model/ConceptPropertyValueLiteral.php 95.55% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@osma osma merged commit 7d7b1bb into master Mar 22, 2023
@osma osma deleted the fix-datetime-to-string-in-containshtml branch March 22, 2023 08:28
@miguelvaara
Copy link
Contributor

miguelvaara commented Mar 22, 2023

Great job @osma 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@osma osma

Labels

bug

Projects

None yet

Milestone

2.17

Development

Successfully merging this pull request may close these issues.

2 participants

@osma @miguelvaara