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

reorder parameters #531

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

yeniatencio
Copy link
Contributor

@yeniatencio yeniatencio commented Oct 22, 2024

Jira

Problem/Motivation

Content-vic and other projects are having behat test failied because of optional parameter $time declared before required parameter $alias_helper.

Fix

Reorder parameters
Initialize the $aliasHelper property of the current class with a service that helps manage alias storage, allowing the class to use this service for its functionality.
assigns the aliasHelper property to the service tide_site.alias_storage_helper to allow the class to use the alias storage.

Related PRs

https://github.com/dpc-sdp/content-vic-gov-au/pull/1725

Screenshots

Screenshot 2024-10-23 at 9 07 19 am

Screenshot 2024-10-24 at 9 21 45 am

TODO

@sharmasahil
Copy link

@yeniatencio I have also tried this fix but din't work in local, so please confirm if you have ran behat tests locally and its successful after this fix.

@yeniatencio
Copy link
Contributor Author

@yeniatencio I have also tried this fix but din't work in local, so please confirm if you have ran behat tests locally and its successful after this fix.

Tested locally and pr branch in content-vic. I have added the pr link and a screenshot to prove that.

parent::__construct($repository, $whitelist, $language_manager, $cache, $time);
$this->aliasHelper = $alias_helper;
$this->aliasHelper = \Drupal::service('tide_site.alias_storage_helper');
Copy link
Contributor

Choose a reason for hiding this comment

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

Using $this->aliasHelper = \Drupal::service('tide_site.alias_storage_helper') isn’t ideal because it bypasses dependency injection, making the code harder to test.

The original error,

      8192: Optional parameter $time declared before required parameter $alias_helper is implicitly treated as a required parameter in docroot/modules/contrib/tide_core/modules/tide_site/src/AliasManager.php line 29 (Drupal\Core\Entity\EntityStorageException)

happened because required parameters must come before optional ones.
try to move AliasStorageHelper $alias_helper before $time

Copy link
Contributor Author

@yeniatencio yeniatencio Oct 24, 2024

Choose a reason for hiding this comment

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

Using $this->aliasHelper = \Drupal::service('tide_site.alias_storage_helper') isn’t ideal because it bypasses dependency injection, making the code harder to test.

The original error,

      8192: Optional parameter $time declared before required parameter $alias_helper is implicitly treated as a required parameter in docroot/modules/contrib/tide_core/modules/tide_site/src/AliasManager.php line 29 (Drupal\Core\Entity\EntityStorageException)

happened because required parameters must come before optional ones. try to move AliasStorageHelper $alias_helper before $time

@vincent-gao , That was my first thought too. You can see in the first commit I have done what you are suggesting and it doesn't fix the issue. 16c32f8

Copy link
Contributor

Choose a reason for hiding this comment

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

😢

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

Successfully merging this pull request may close these issues.

3 participants