TaxonomyIndexDepth is a bit different because it has the taxonomy_index table unlike other entities.
Maybe we could utilize this instead of field references.
See patch.
There is some redundancy ( checking all related terms ) in here which could have impact on performance.
In our cases this does not seem to give any issues.
Looked at it again but still can't figure out how this is feasible, therefor placing it in "bug report" section.
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.
Willempje2 → created an issue.
Willempje2 → created an issue.
Willempje2 → created an issue.
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...
Ah bummer... Thanks for the review though!
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.
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.
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?
Priority change as per Issue priorities. →
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.
Patch #25 works for me.
Unfortunately patch #27 does not, only works with the "!important" added to the style.
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
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.
@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
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... →
Willempje2 → created an issue.
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.
Take a look at:
https://www.drupal.org/project/libraries/issues/3342032
🐛
Undefined method getImplementations()
Fixed
Seems to be same problem.
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.
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']];