Issue #3432191: Fix PHPStan warnings from Drupal upgrade #3825
Conversation
vcsvinicius
added
status: needs review
|
Thanks for contributing towards Open Social! A maintainer from the @goalgorilla/maintainers group might not review all changes from all teams/contributors. Please don't be discouraged if it takes a while. In the meantime, we have some automated checks running and it might be that you will see our comments with some tips or requests to speed up the review process. 😊 |
vcsvinicius
changed the title
vcsvinicius
force-pushed
the
feature/3432191/fix-phpstan-warnings
branch
from
8c32e2f
to
9feada7
Compare
| @@ -94,7 +94,7 @@ public function enabled() { | |||
| $usage_data_settings = $config->get('usage_data'); | |||
|
|
|||
| $plugin_definition = $this->getPluginDefinition(); | |||
| if (in_array($plugin_definition['setting'], $usage_data_settings)) { | |||
| if (in_array($plugin_definition['setting'] ?? [], $usage_data_settings)) { | |||
There was a problem hiding this comment.
This looks a bit weird, because usually you check if a string is inside an array with the method.
But apart from that, why would you run the method with an empty array because you already know its would not be in the array.
| $plugin_icon = $module_path . '/assets/icons/' . $this->pluginDefinition['id'] . '.svg'; | ||
| $plugin_icon = empty($this->pluginDefinition['id']) | ||
| ? $default_icon | ||
| : $module_path . '/assets/icons/' . $this->pluginDefinition['id'] . '.svg'; | ||
|
|
||
| return '/' . (file_exists($plugin_icon) ? $plugin_icon : $default_icon); |
There was a problem hiding this comment.
I think we need to rewrite this also now, because in your case plugin_icon is never empty anymore.
There was a problem hiding this comment.
In the function return we are checking if file-exist and it can happen yet, but I can put together with previous if, what do you think?
There was a problem hiding this comment.
yes, in the old situation if the file exist we return $plugin_icon, and otherwise $default_icon.
But in the new one, it will never return $default_icon right? as we put that value in the $plugin_icon, so it will always return this $plugin_icon as it will always exist? Or is that a situation i'm not seeing where the return will return $default_icon still?
There was a problem hiding this comment.
If the image continue existing at database, but locally the file is missing, then we will use the default-icon.
There was a problem hiding this comment.
Not sure if I understand correctly, because you are now putting the default_icon in the plugin_icon it will always exist right? I don't see how the local file can not exist in our scenario.
$plugin_icon = empty($this->pluginDefinition['id'])
? $module_path . '/assets/icons/default-calendar.svg'
: $module_path . '/assets/icons/' . $this->pluginDefinition['id'] . '.svg';
return '/' . $plugin_icon;
| @@ -106,17 +108,17 @@ public function getEventDates(NodeInterface $node) { | |||
| $date_time = []; | |||
|
|
|||
| // Set formats for event dates. | |||
| $format = $this->pluginDefinition['dateFormat']; | |||
| $format = $this->pluginDefinition['dateFormat'] ?? 'Ymd\THis'; | |||
There was a problem hiding this comment.
For me it feels a bit weird to be presetting the dates here if it's empty as we are then trying to hide an error/misconfiguration.
I would expect that if the dateFormat is not available at this moment then something is wrong with the platform and we should return an error or something.
There was a problem hiding this comment.
I believe our code are safe to these data never arrived empty here, because never had an error here, but, I thought in a default value to simplify the solution. I opened a discuss about default values here: https://opensocial.slack.com/archives/G01CXDZ0FCG/p1710789447543369 and I got from here: html/profiles/contrib/social/modules/social_features/social_event/modules/social_event_addtocal/src/Annotation/SocialAddToCalendar.php
There was a problem hiding this comment.
Hmm looking at the slack conversation, if the default are already set, do we need to retrieve it like this? Because it would mean we need to change it in 2 places if the defaults change?
There was a problem hiding this comment.
I thougth in two options:
- Create an excepetion for this module and throw them, because it is a function called by front, so I believe this probably is the best way;
- Create a constant to save the default values and it will be saved in one place.
What do you think?
There was a problem hiding this comment.
I believe 1 can work yes
|
|
||
| // Convert date to correct format. | ||
| // Set dates array. | ||
| if ($all_day) { | ||
| $date_time['start'] = $start_date->format($all_day_format); | ||
| $end_date->modify($this->pluginDefinition['endDateModification']); | ||
| $end_date->modify($this->pluginDefinition['endDateModification'] ?? '+ 1 day'); |
There was a problem hiding this comment.
I'm also a bit lost about what this exactly does. If an end date is empty we return 1 day?
vcsvinicius
force-pushed
the
feature/3432191/fix-phpstan-warnings
branch
from
9feada7
to
25892ea
Compare
Problem
The last Drupal upgrade start to throw some PHPStan warnings and to continue with the upgrade these warnings was added to be ignored by PHPStan.
The PHPStan ignore added can be check here: https://github.com/goalgorilla/open_social/pull/3806/commits/14894189009...
Solution
Fix these warnings and remove it from PHPStan ignore.
Issue tracker
PROD-28482
#3432191
Theme issue tracker
N/A
How to test
N/A
Definition of done
Before merge
After merge
Screenshots
N/A
Release notes
Fixed errors ignored from last Drupal upgrade.
Change Record
N/A
Translations
N/A