-
Notifications
You must be signed in to change notification settings - Fork 12
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
Feature/drupal 10 compatibility #98
Feature/drupal 10 compatibility #98
Conversation
gitmonkee
commented
Aug 21, 2023
- Fix Drupal 10 compatibility issues.
- Upgrade js dependencies
src/DvfHelpers.php
Outdated
@@ -50,8 +48,7 @@ public function transformMachineName($string) { | |||
* The link. | |||
*/ | |||
public function getHelpPageLink($template_name) { | |||
$link = Link::fromTextAndUrl('Help', Url::fromUserInput($this->helpPageBasePath . $template_name)); | |||
return new FormattableMarkup('<span class="dvf-admin-popup">' . $link->toString() . ' ⧉</span>', []); | |||
return new FormattableMarkup('<span class="dvf-admin-popup"><a href="/dvf/help/' . $template_name . '">Help</a> ⧉</span>', []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to add template variables ? Not a biggy
return new FormattableMarkup('<span class="dvf-admin-popup"><a href="/dvf/help/' . $template_name . '">Help</a> ⧉</span>', []); | |
return new FormattableMarkup('<span class="dvf-admin-popup"><a href="/dvf/help/@template_name'">Help</a> ⧉</span>', ['@template_name' => $template_name]); |
src/DvfHelpers.php
Outdated
@@ -50,8 +48,7 @@ public function transformMachineName($string) { | |||
* The link. | |||
*/ | |||
public function getHelpPageLink($template_name) { | |||
$link = Link::fromTextAndUrl('Help', Url::fromUserInput($this->helpPageBasePath . $template_name)); | |||
return new FormattableMarkup('<span class="dvf-admin-popup">' . $link->toString() . ' ⧉</span>', []); | |||
return new FormattableMarkup('<span class="dvf-admin-popup"><a href="/dvf/help/' . $template_name . '">Help</a> ⧉</span>', []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also just a suggestion if you want to use render array instead of markup. https://www.drupal.org/docs/drupal-apis/render-api/render-arrays#:~:text=This%20provides%20enormous%20flexibility%20in,Drupal's%20equivalent%20of%20the%20DOM.
@@ -197,42 +197,42 @@ protected function getIterator() { | |||
/** | |||
* {@inheritdoc} | |||
*/ | |||
public function current() { | |||
public function current(): mixed { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lem-doghouse, I'm curious about what else could be returned here. Is there a way to avoid mixed return type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @amjad1233 ,
An alternative solution is to add the #[\ReturnTypeWillChange] attribute. It's worth noting that the current() method in the Iterator's Interface is marked with #[TentativeType], indicating that the return type can change dynamically based on runtime conditions.
If you find it suitable, I'm more than willing to make adjustments by replacing the mixed return type and integrating the #[\ReturnTypeWillChange] attribute instead. Please let me know your thoughts on this matter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh cool thanks for the run down just checked the interface... Let's leave it as it is for now... I think #[\ReturnTypeWillChange]
has compatibility issues with older PHP from memory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. nice work @lem-doghouse 🏆
9231d94
to
370bcf7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good Lem, Just the usage of once should probably be tweaked
js/dvfTables.js
Outdated
@@ -7,7 +7,7 @@ | |||
*/ | |||
Drupal.behaviors.dvfTables = { | |||
attach: function (context) { | |||
$('[data-dvftables]', context).once('dvf-tables').each(function () { | |||
$(once('dvf-tables', '[data-dvftables]', context)).each(function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gitmonkee I don't think this is the correct usage of once
see https://www.drupal.org/docs/drupal-apis/javascript-api/javascript-api-overview#s-example-using-drupalbehaviors
once('dvfTables', '[data-dvftables]', context).forEach(
function (element) {
$table = $(element);
...
}
);
Obviously still works but not sure if it is optimal
80009ea
to
21a0306
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff @gitmonkee !
👋 Hey @jez500! Looking after a few GovCMS D10 upgrades at the moment, do you expect further change required for a final D10 release, or relatively safe to start using the |