🇳🇱Netherlands @Willempje2

Account created on 21 August 2017, almost 7 years ago
#

Recent comments

🇳🇱Netherlands Willempje2

Looked at it again but still can't figure out how this is feasible, therefor placing it in "bug report" section.

🇳🇱Netherlands Willempje2

Your if statement seems to be inverted.
Seems more sensible to skip if cloneable entities are empty or if target entity is not present in the array.

🇳🇱Netherlands Willempje2

Not sure if i need to create a new issue for this but my guess would be that "blockSubmit($form)" on line 53 should be changed as well.

https://git.drupalcode.org/project/webform/-/blame/6.2.x/js/webform.elem...

🇳🇱Netherlands Willempje2

Ah bummer... Thanks for the review though!

🇳🇱Netherlands Willempje2

I still agree that using the Drupal widget in this way is a form of misuse, although perhaps not to the extent you have in mind (or maybe I'm overlooking something, in which case I sincerely apologize).

Allow me to describe a use case for when we use this module:
Consider a content/node type named "festival event." These festival events have various fields, including a datetime field for the start of the event and another for the end of the event (which could be days or hours apart). On the website, we aim to highlight events that are currently ongoing within a view. This is achieved by adding a class based on a set of "if" statements. Unfortunately, these if statements are never checked once the page is stored in the cache.

By applying this widget to the existing "festival event start date" field, the page cache automatically invalidates once the start date has passed. This allows the if statements to be reevaluated.
Different fields utilizing this widget will indeed trigger multiple cache invalidations at different times, and this is intentional. (Note that not all dateTime fields necessarily need to utilize this widget.)

In this instance, I don't want to alter the input or the intended purpose (to some extent) of this field, and I also don't want to directly modify the output.

Hope this helps.

🇳🇱Netherlands Willempje2

I understand, and thank you for the clarification. I genuinely hope that this doesn't become a deal-breaker for the module. For us, the practical benefit of changing the widget instead of the field type outweighs the downside of misusing the widget in a way that alters the output.

Regarding the second point:
I don't believe it's possible to specify the cache tag any further. If it is, I'm not aware of what difference it would make during invalidation. (Unless cache tags can also accept display types, but I'm not familiar with such a feature if it exists.)
If multiple fields of the same type have this widget, the value of the datetime field would likely be different, resulting in invalidation at different times.

🇳🇱Netherlands Willempje2

Translatable error message:
This has been updated.

Redundant config get:
This has been updated.

Package in info.yml:
This has been removed.

Wrong use of field widget:
I understand the issue, but I'm struggling to find an appropriate place for this change and/or a practical solution for implementing it elsewhere. It doesn't affect the output of the field, and I don't want to change the type because using the widget allows implementation on existing fields.

Consider using multiple fields:
Unfortunately, I don't quite understand this comment. The 'hook_entity_update' loops over all fields using this widget and schedules separate expiration items. Could you elaborate on this?

🇳🇱Netherlands Willempje2

Example form
This was also mentioned by kevin.brocatus but up until now i did not have the time to implement this fully.
1.0.x now includes a useful settings form.

DateTimeDefaultWidget
Base class is now extended instead of the internal class.

🇳🇱Netherlands Willempje2

Patch #25 works for me.
Unfortunately patch #27 does not, only works with the "!important" added to the style.

🇳🇱Netherlands Willempje2

Not sure if link works or if you want to support PHP 7 but does seems to be true for PHP 7.4.33
https://3v4l.org/QMF0j#v7.4.33

🇳🇱Netherlands Willempje2

As mentioned by kiwimind this seems to be related to this issue:
https://www.drupal.org/project/drupal/issues/3217868 🐛 Claro theme + field_group seems to hide content of dropbutton Needs work

Patch in issues worked for me.

🇳🇱Netherlands Willempje2

@kevin.brocatus thanks for the review!
@shashank5563 Did not know this was a requirement, thanks for letting me know!

PHPCS issues.
This is now updated.

Example method.
Removed, readme has some examples so it was already kinda unnecessary.

Camel case and snake case.
This is now updated.

hook_entity_postsave
This is now updated.

empty check as the result of $ct_expire->getExpired();
This is now updated.

SettingsForm.php
Unless this leads to a security issue i would prefer to let it in.
Like you guessed i would like to make use of this in the near future, seems silly to remove it now and add it back later.

getExpired() function
As far as i know using the full expression like this is faster.
Unless other arguments are given i prefer to keep it as is.

Branch
Default branch changed to 1.0.x

🇳🇱Netherlands Willempje2

Just going through list given in the "PAReview: Review Template" and check what could be applicable to this module:

Code long/complex enough for review
Application has more than 120 lines and 5 function or class method definitions

Secure code
Twig templates correctly used
Placeholders are correctly used for database storage

Javascript (jQuery) and Drupal.checkPlain()
This might be issue, my jQuery knowledge is limited but there seems to be values outputted in DOM that are not sanitized.
See: https://git.drupalcode.org/project/virtual_select/-/blob/1.0.x/js/virtua...
And: https://www.drupal.org/docs/security-in-drupal/writing-secure-code-for-d...

🇳🇱Netherlands Willempje2

So i resolved some of the issues mentioned in comments from apaderno in separate branch fetched from the PR from bharath-kondeti.
These changes are now merged and available in new tag.

If someone is willing to do another code snif that would be much appreciated.
For now i will set the status to fixed and try to credit everyone involved.

As you may notice i am a bit new to this, so if i forgot anything (in crediting or in the workflow) please let me know.

🇳🇱Netherlands Willempje2

It seems someone beat me to the punch.
See: https://www.drupal.org/project/drupal/issues/3312001
Although i think my solution might be more elegant.

🇳🇱Netherlands Willempje2

I think you should change the value the other way around.
Do not change $_SERVER['REMOTE_ADDR'] but use the remote REMOTE_ADDR to set the reverse_proxy_addresses.

Like so: $settings['reverse_proxy_addresses'] = [$_SERVER['REMOTE_ADDR']];

Production build 0.69.0 2024