Hi dieterholvoet,
I'm happy to accept your offer to maintain the module together. I don't have sufficient time to react as fast as I would want. So any help is welcome. I added you as maintainer.
Tested this on a Drupal 10.3 install with PHP 8.3 and everything works.
tim-diels → changed the visibility of the branch 2.0.x to hidden.
Postponing this until the other modules have the patches accepted.
tim-diels → created an issue.
Some misconfiguration on my end
I'm not sure what the max allowed value should or could be. If i'm searching I can find an issue with MySQL https://bugs.mysql.com/bug.php?id=103992
The number 9223372036854775807 seems acceptable by Drupal and 9223372036854775808 not. So it does seem its the function FILTER_VALIDATE_INT that messes this up. But I don't want to remove this one. Is there another function maybe we can use to validate if this is a unsigned bigint?
@lazzyvn You can't just add new things to an issue. Please create a different issue to add the logo please (and remove from here) and can you create a MR to review?
@lazzyvn this should be handled in a different issue. This issue is only for correcting the category. I created a new issue 📌 Update annotations to PHP attributes Active to handle the change of the annotation to PHP attributes. You can add the changes for the annotation to PHP attributes there, thanks.
tim-diels → created an issue.
Can not reproduce the error. So seems it is fixed. Closing this issue, but feel free to create a new issue if there is still a problem.
Created a release 2.0.0
So if I understand correctly, the 2 modules can work next to each other but should not do anything when Guardian is activated for an account? Would be good to have some kind of test or at least someone else can review this?
Would be good to see if this issue is still active or not.
tim-diels → changed the visibility of the branch 3478053-upgrade-to-core to active.
tim-diels → changed the visibility of the branch 3478053-upgrade-to-core to hidden.
I'll revert the support for Drupal core 11.x on the 8.x-1.x branch and create a new branch to support 11.x.
Ok let’s seeing we can make a test with this suggested mask setting or manual test. I’ll come back to this. If anyone in meantime has time before me, feel free to try it out and provide feedback.
So as I read it the mask setting is 999,999,999,999.99
If not, let us know.
Had probably some wrong configuration or extra configuration. Will close this for now.
Rebasing. Will try to find some time to add tests. Or if anyone else wants to give this a go?
I'm sorry but I have first accepted all coding standard issues. So this issue will need rebasing to continue.
Can we have tests for this issue to show its failing?
@sourabhsisodia_ I'm sorry but I have first accepted all coding standard issues. So this issue will need rebasing to continue. I guess the tests will fail also or at least needs updating.
@sayan_k_dutta Thanks for the work and follow up. Appreciated!
@dhruv.mittal thanks for the work and the follow up. Appreciated!
tim-diels → created an issue.
@kul.pratap thanks for the good work and follow up. Appreciated!
Needs rebasing.
Needs rebasing.
@avpaderno It was in the issue queue for 2 weeks. It could have a better title yes. But can we move it back to the "Drupal.org project ownership" issue queue? Not sure what the "Postponed (maintainer needs more info)" is for?
New error appeared
------ -----------------------------------------------------------------
Line src/Form/SettingsForm.php
------ -----------------------------------------------------------------
266 Call to deprecated method clearCachedDefinitions() of interface
Drupal\Core\Asset\LibraryDiscoveryInterface:
in drupal:11.1.0 and is removed from drupal:12.0.0. Use
LibraryDiscoveryCollector::clear() instead.
------ -----------------------------------------------------------------
Change notice:
LibraryDiscovery class is deprecated →
📌
Deprecate LibraryDiscovery and use LibraryDiscoveryCollector instead
Fixed
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Asset%21L...
@todo Use the deprecated method helper.
DeprecationHelper helps modules support multiple versions of core →
Could also autowire the drush services.
Waited for 2 weeks without any response. Moving this over accordingly.
I'm glad you continue to help. For me everything is ok now. Test seems to pass correctly also.
So thank you for the work here as this is very helpful for the other issues to get accepted or work on!
Hi @anish.ir, Thanks for the follow up.
But why are you creating tests that do nothing like the examples? I do understand it shows it works, but is not needed.
I'm also not getting the extra MaskKernelTest.php that only has the exampled and why LibraryInfoAlterTest.php is extending this instead of the MaskKernelTestBase.php. And the same for MaskUnitTest.php. Can you elaborate on why you did this?
Other changes are indeed ok. So only the changes for the abstract needs to be looked at. Great work!
Ok the changes done in tests/src/Unit/ElementHelperTest.php seems correct so you can keep those. Very good work!
Need to review the others.
The change of just removing the abstract is not correct. Please see
https://www.drupal.org/project/drupal/issues/2973127 →
for more info on this.
So I would suggest changing the base tests to include *Base and change the other files extending these base classes.
- MaskKernelTestBase.php
- MaskUnitTestBase.php
And then start from there to fix the tests. So maybe revert the changes or start fresh?
Because I'm not sure what you changed to get everything working...
You should focus on fixing one task at a time to make it easier to follow up. You can create separate tasks to fix other things and I would suggest on doing so. I will then make some time to review tasks when they are split to simpler tasks.
Did you follow the readme? As the error explains the library is not found and that is not automatically installed. See https://git.drupalcode.org/project/tablesorter#installation
As I had the same error, but following the guide solved the issue for me.
Marking this as "Closed (works as designed)".
Could you provide some more information on what you actually try to achieve and how? I think it is not very clear at this moment what you want to do and how.
tim-diels → created an issue.
If you want a maintainer to review your question I would suggest adding examples on what you did for this module. Give some examples on what you fixed/reviewed/assisted on.
Additionally I would add some examples to where you assisted other modules. This would help in the decision process of the reviewers.
"The module has no menu or modifiable settings. There is no configuration. When enabled, the module will prevent the links from appearing. To get the links back, disable the module and clear caches."
This text is wrong. As @erutan mentioned there is a config route and it should be explained.
I would also suggest removing the maintainers section as this could be handled by the composer.json provided in the
✨
Sort option would be wonderful
Needs review
@erutan I do like your suggestion. But can't put time in this now to completely refactor it to be a submodule. If a maintainer could assist in deciding what would be the best approach, then we can see what we need to do to get this finished.
I understand that there is no proper release for tablesorter. I'll try to get some time to get a proper release there. I would suggest not yet adding this to a release until there is a proper release of tablesorter.
The CSS I did not add as I think this is not necessary as a theme could handle this? Any opinions on that?
Setting this to needs review as in my eyes it seems complete now.
We can also suggest installing the tablesorter in a composer file...
tim-diels → created an issue.
The code works for me, only no visual for sorting. Is this something that is needed for the tablesorter module itself or it could be added here as done in the 7.x branch. The tablesorter said is is something the theme should fix. So maybe just some documentation and just the cursor set to pointer?
Setting to needs work for the css.
Added patch from #17 to the latest codebase so going to hide all other patches.
I think a nice follow up would be a setting to allow to specify what columns should be sortable as the library supports this. But that could be a separate issue so this can go in as is and the change is not to big.
Patch from #17 and patch from #21 are quiet different.
What is the expected approach here to have table sorting added? I see the module uses the Tablesorter → module for the 7.x version and that was the same as with patch from #17 but the person in #21 decided on itself that they wanted to go a different approach without explaining why.
I'm more of a fan of using the Tablesorter → module as this is also what the maintainer proposed on using. So can we have a concensus on using this approach?
Thanks for the great effort here @dhruv.mittal! Appreciated!
Before I can accept code changes we should get
📌
Fix tests
Active
fixed so we know we're not breaking our automatic tests.
I shall do a manual review also when we can see our tests don't fail already.
This should be done for the 2.1.x branch. I know the default was 2.0.x but I don't have the correct right to change. I'll ask the maintainer to change this.
Only one error remains
------ -----------------------------------------
Line src/Plugin/FieldWidgetPluginManager.php
------ -----------------------------------------
14 Plugin definitions cannot be altered.
------ -----------------------------------------
[ERROR] Found 1 error
I saw another issue with probably the solution https://www.drupal.org/project/message_notify/issues/3414123 📌 Issues reported by phpStan Active
This should be done for the 2.1.x branch. I know the default was 2.0.x but I don't have the correct right to change. I'll ask the maintainer to change this.
There are still 2 errors which can be fixed really easily. Why do you ask a maintainer to fix the remaining issues? You could ask some guidance in here or on slack on how to solve the remaining issues? I would love to see some more effort before I'm willing to give some credits as this seems an easy way to get a credit and shift the problem to someone else.
This should be done for the 2.1.x branch. I know the default was 2.0.x but I don't have the correct right to change. I'll ask the maintainer to change this.
Thanks kul.pratap for the work done. Looks good!
tim-diels → created an issue. See original summary → .
This should be done for the 2.1.x branch. I know the default was 2.0.x but I don't have the correct right to change. I'll ask the maintainer to change this.
This looks really good. Will need to find time to test this. If in meantime someone else could test this, would be great. Please also change the target version to 2.1.x
tim-diels → created an issue.
tim-diels → created an issue.
tim-diels → created an issue.
tim-diels → created an issue.
Added GitLab CI file. We need to create follow up tasks for the warnings and errors.
tim-diels → created an issue.
Thank you so much for pointing this out! I tested this and found that it is better to have the checkboxes as a boolean. So changed the datatype for them to boolean.
tim-diels → changed the visibility of the branch 3431819-automated-drupal-11 to hidden.
tim-diels → changed the visibility of the branch 2.0.x to hidden.
tim-diels → made their first commit to this issue’s fork.
You should look at Entity field condition → as that provides this condition out of the box for you for nodes. It would be fairly easy to look how this is done and do this yourself for taxonomy and provide a feature request there. Maybe this can be added as documentation and this can be closed as works as designed?
Adjusted the empty bundle option and reverted the empty field option
In my opinion the option danflanagan8 added is what should be added. Let the end user choose a bundle or any bundle. Therefor I created a MR with that patch and hidden all other patches.
But I follow that the select options need some update to reflect the correct text shown. I'll pick this up.
tim-diels → made their first commit to this issue’s fork.