Cascadia
Account created on 8 November 2007, over 17 years ago
#

Merge Requests

More

Recent comments

🇺🇸United States tr Cascadia

Please try out the fix in that other issue, then comment in that other issue to let us know whether that fixed the problem for you.

🇺🇸United States tr Cascadia

Please try out the fix in that other issue, then comment in that other issue to let us know whether that fixed the problem for you.

🇺🇸United States tr Cascadia

A problem with the initial commit was reported in #29 so the issue was reopened to fix that problem.
2.2.1 has MR!56, the next release will also have MR!57. You can use the patch from MR!57 or use the -dev release if you want the fix immediately.

🇺🇸United States tr Cascadia

This was already fixed in Conflicting name with other element(s) Needs review .

🇺🇸United States tr Cascadia

This was already fixed in Conflicting name with other element(s) Needs review .

🇺🇸United States tr Cascadia

That's good enough for me.

🇺🇸United States tr Cascadia

This appears to be the same as Conflicting name with other element(s) Needs review

@jackfoust
@klemendev

If this is important to you, please try out the patch in Conflicting name with other element(s) Needs review and report in that issue whether it fixes the problem for you or not.

🇺🇸United States tr Cascadia

Hey @loze, maybe you can comment on 📌 JavaScript coding standards Active ? Try out the patch and let me know if it works for you?

I did the work, but 6 months without a single comment from the community makes me think that no one cares about this module. If this module is important to you, maybe you can spend a few minutes giving feedback on the proposed fixes?

🇺🇸United States tr Cascadia

@pcate: If you apply the patch from MR!57, does it fix the issue on your site?

@joe huggans: I think the MR and the test are fine as they are, if @pcate can confirm that it fixes the problem on his site.

🇺🇸United States tr Cascadia

Drupal 7 is no longer supported, and honeypot.css doesn't exist in Honeypot 2.x.

🇺🇸United States tr Cascadia

Added tests to make sure this keeps working!

🇺🇸United States tr Cascadia

MR should be good now. Can you modify the test too, to check for this case?

🇺🇸United States tr Cascadia

I followed your steps, and my .yml looks the same as yours.

But I don't see this error.

However, I don't have the exact same versions as you do - I am using Drupal 11.0 and tecnickcom/tc-lib-barcode: 2.4.0

I'm assuming this is a new install of the Barcodes module, and not a site where you have upgraded from a previous version of Barcodes? Internally, width and height used to be strings, but that was fixed almost two years ago in release 2.0.5. However, if you upgraded or imported your configuration from an older installation then that might account for the wrong type.

When I get a chance I will try to write a test case that follows your steps.

I noticed a strange behavior in views: if i change a setting of field EAN13 and preview without saving the view, bug appears, if i save view before previewing, there's no bug.

I think this happens for all field formatters in views. If you now go and change the color or the barcode type - the preview will still use the old settings until the view is saved. You issue seems to be that initially the width and height are stored in the Field Formatter settings as strings, and only get changed to integers when the formatter settings are saved.

I am strongly opposed to just casting things - all that does is hide the typing error. Ensuring we are passing around the data that we think we are passing around is important in order to make this code better. As far as I can tell width and height are being initialized as integers, input as integers on the settings form, and passed around as integers everywhere, so if they are being changed to string at some point I want to know where and correct it at that point.

🇺🇸United States tr Cascadia

"I think such a big change deserves a change record"
"I can't find a change record for this. If an existing one covers also this change, could it be linked?"

I agree. No change records mention ContentEntityType, for example, or any other of the types changed (in core) by this issue.

It is unclear to me whether I can use attributes for these types in 10.3 like with most of the other annotation->attribute conversions, or whether using attributes for these types is only supported in 11.1+. Usually that information is found in the change record.

The only change records that talk about annotation->attribute are:
Plugins converted from Annotations to Attributes in 10.3.0
and
Plugin implementations should use PHP attributes instead of annotations
The latter says Drupal 10.2, warns that not everything is done yet, and claims it will be updated with additional plugin types as they are converted in core. That hasn't happened.

Neither of these change records mentions any of the types converted by this issue.

🇺🇸United States tr Cascadia

"Fix is in the master now!"

It seems this was probably fixed long ago.

Regardless, Drupal 7 is no longer supported, and the Radioactivity code in Drupal 8+ is significantly changed. There have been no further reports of a problem like this for the past 13 years.

🇺🇸United States tr Cascadia

No one ever responded to #8. Perhaps #2480667: Avoid excessive database activity when using field_sql_storage fields really did fix the issue, because there have been no further posts about this bug in 10 years.

Regardless, Drupal 7 is no longer supported, and entities/fields are handled in a much different way in Drupal 8+. If this is still a problem with the current version of Radioactivity, please open a new issue with instructions for how to reproduce the problem.

🇺🇸United States tr Cascadia

tr created an issue.

🇺🇸United States tr Cascadia

Alternative storage is supported for Radioactivity entities by default in Drupal core 8+.

Drupal 7 is no longer supported.

🇺🇸United States tr Cascadia

Drupal 7 is no longer supported, and there have been no further reports of this bug in the past almost 7 years.

This code doesn't exist in current versions of Radioactivity. If you encounter a similar problem in Radioactivity 4.x please open a new issue with instructions for how to reproduce the problem.

🇺🇸United States tr Cascadia

Drupal 7 is not longer supported and this configuration file does not exist in current versions of radioactivity.

🇺🇸United States tr Cascadia

tr created an issue.

🇺🇸United States tr Cascadia

tr changed the visibility of the branch 4.0.x to hidden.

🇺🇸United States tr Cascadia

Closing this as a duplicate of the two linked "related issues"

🇺🇸United States tr Cascadia

Outdated and no longer relevant since we switched to GitLab CI last year.

🇺🇸United States tr Cascadia

Drupal 7 is no longer supported.

🇺🇸United States tr Cascadia

Drupal 7 is no longer supported.

The root cause of this problem with class loading in systems (both PHP and Drupal) that were not really designed for object-oriented programming. But the class loader in PHP and Drupal has been considerably overhauled in newer versions of PHP and Drupal, and we no longer experience any problems like this.

🇺🇸United States tr Cascadia

Drupal 7 is no longer supported.

The root cause of this problem with class loading in systems (both PHP and Drupal) that were not really designed for object-oriented programming. But the class loader in PHP and Drupal has been considerably overhauled in newer versions of PHP and Drupal, and we no longer experience any problems like this.

🇺🇸United States tr Cascadia

Drupal 7 is no longer supported, and this problem doesn't exist in the current versions of the Radioactivity module.

🇺🇸United States tr Cascadia

Drupal 7 is obsolete, and the Radioactivity module for the current versions of Drupal (10, 11) passes all coding standards tests without errors reported.

🇺🇸United States tr Cascadia

tr created an issue.

🇺🇸United States tr Cascadia

Mmm ... cascading failures. I might have to back off and only use strict types for part of the code for the time being, because fixing all the tests might prove too time consuming for now.

🇺🇸United States tr Cascadia

And the failed test shows exactly why it is so important to use strict typing (and why it is important to have working tests like we now do!) - somewhere in the code we have "energy" being passed around as a string instead of a float. That error was being hidden due to PHP type coercion taking place when strict typing was disabled.

🇺🇸United States tr Cascadia

tr created an issue.

🇺🇸United States tr Cascadia

The newly-changed "Remaining tasks" in the issue summary are not described anywhere, and are not really remaining tasks but rather feature requirements?

I think the first step to make progress here needs to be to open up an MR with a concrete set of changes that can be tested and discussed - there is currently only an old patch file that is horribly out-of-date - for example it doesn't even account for the core change from "context" to "context_definitions" made back in 2019, or the change from Annotations to Attributes made in core last year.

But to do that we need a well-written issue summary stating exactly what the goal of this issue is. The "Problems" section and "Proposed resolution" section still seem relevant (and are still addressed by what was done in Rules), but the "Tasks" and "API changes" sections need clarity.

As I said in #126, I'm willing to offer an MR extracted from Rules, but I would like a little clarity here on what people are looking for from Core.

🇺🇸United States tr Cascadia

As a reminder, Rules *still* has context-aware actions working, along the lines being proposed here, with extensive test cases.

This issue was opened because the development of Rules in the Drupal 7->8 transition identified functionality that was needed for Rules and would be useful to core as well. Back then @fago and @klausi were actively involved in developing Rules and were leveraging that work to contribute to the still-evolving core Drupal 8, and this issue was part of that effort.

The patches in here contain a lot of (old) code developed for Rules. This issue seems to have lost traction for inclusion into core when @fago stopped pushing this issue back in 2015 and put the needed functionality directly into Rules rather than wait for core to change.

As prophesized in #9, RulesAction plugins are now a fork of the core Action plugins, made context-aware. If anyone is interested, the commit that did this in the Rules repository was 55fe6db1, and it is similar to what was offered in #75 above. It's a shallow fork, done only because subclassing to get the needed functionality was not possible.

Rules implemented the functionality discussed here long ago. It works. And that functionality (and tests) have been maintained, improved, and ported to Drupal 9, 10, and 11 and is currently being tested weekly on GitLab CI. Rules and Rules-related modules supply hundreds of working examples of these plugins, with many examples and lots of documentation.

I would suggest to anyone who has a need / use case for context-aware actions that you try the Rules implementation as an archetype of how this can be implemented. There's a huge amount of commonality between what Rules has already done and what is being discussed here, precisely because this issue was opened to enable functionality needed by Rules.

I would be happy to help move this Rules functionality into an MR for core, but it would help to first have some feedback on which features should be in core, which should remain in Rules, and what is missing from Rules that core would need.

🇺🇸United States tr Cascadia

Here's the enhanced test. Maybe could be simplified a bit, but this seems to work locally.

🇺🇸United States tr Cascadia

tr created an issue.

🇺🇸United States tr Cascadia

Thanks for helping out!

This is my first time looking at this patch. In general I'm in favor of this change, and I agree that the entities should be managed in the field type, not in the widget. I especially like all the new tests - that really helps to define the expected behavior of this code and ensure that we don't break the expected behavior with future changes to the codebase.

But now that all the module tests for 4.1.x branch are passing, I want to make sure we don't introduce new test failures with this MR. So what I would like to see here is to have all the test warnings/errors/failures fixed in the MR before I do a detailed review and commit this. Can you work on the MR to fix the problems with the tests?

🇺🇸United States tr Cascadia

tr changed the visibility of the branch 3325229-api-create to hidden.

🇺🇸United States tr Cascadia

tr changed the visibility of the branch 4.0.x to hidden.

🇺🇸United States tr Cascadia

I believe the original post motivation is no longer an issue. Nowadays, nearly all DBs treat the VARCHAR() length as characters, not bytes. Perhaps in the old days when this issue was posted, it was bytes.

I believe @vidorado is correct here.

I haven't done the research so I can't say for certain, but I believe all current DBs supported by Drupal use (or can use) a utf8 character set. In which case VARCHAR(n) means n characters, not n bytes. And yes, that would hold true for all fields, not just comment title fields.

In the case of MySQL, this change (from bytes to characters) took place between MySQL 4 and 5. We are now up to MySQL 8.

Drupal installation instructions say that utf8 encoding should be used when configuring the database ( https://www.drupal.org/docs/getting-started/installing-drupal/create-a-d... ). I don't know if this enforced at the core level or whether it is just assumed - regardless, I expect if you set pure ascii encoding on your database then Drupal core would fail in many places.

It would probably be a good idea to verify that utf8 encoding in the database is enforced by core, and perhaps to clarify the Drupal documentation about defining DB schemas to point out that VARCHAR() refers to characters not bytes. Note, https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Database%... does make a distinction between 'varchar' and 'varchar_ascii' in a schema definition, but doesn't make it explicit that 'varchar' refers to characters not bytes. Not sure it matters in most cases anymore, but IMO it's better to be explicit to dispel any doubts given that there is a long history in Drupal (and PHP, and SQL, and all code) of poor support for non-ascii character sets.

🇺🇸United States tr Cascadia

hook_honeypot_add_form_protection() and hook_honeypot_form_protections_alter() were added to the Honeypot module by #1778296: add hook in honeypot_add_form_protection . That was back in 2012, in the Drupal 6 version!

When trying to use the HOOK honeypot_add_form_protection the values are not given by reference so you can't edit them

This is true. However, the hook signature is currently the same as it has always been, and has never changed. I don't think this can be considered a bug, because that appears to have been intentional. Regardless, that has always been the documented behavior ever since these hooks were first defined.

The problem with your proposed change is that swapping the order of the arguments will break any code that is relying on the "old" order. And allowing the form to be modified inside of hook_honeypot_add_form_protection() is also an API change, which may also break any code using this hook. To make this change properly, there would have to be a BC layer that accepted both the old and new parameter order, and also a @deprecated notice when this hook is called with the "old" order, to inform users of this hook that the parameter ordering has changed. There would also have to be tests for this change.

I guess the question I would ask is, why do you want to alter the form in this hook? And why wouldn't you use hook_form_alter()or some other method instead? I would be more inclined to accept a new, more logical and useful hook or hooks instead of trying to change the old hook.

🇺🇸United States tr Cascadia

Still need the information I asked for above and instructions for how to reproduce this error.

🇺🇸United States tr Cascadia

No, this was never resolved, the issue is still open. If you can help out perhaps we can get this finished.

🇺🇸United States tr Cascadia

tr created an issue.

🇺🇸United States tr Cascadia

I guess not? More likely though is that there aren't a lot of people who answer questions in the issue queues, so this is not the best place to look for help.

Regardless, there's nothing special about Commerce that would prevent you from doing that, that is one of the use cases for this module after all.

If you run across a tutorial for how to do that please add it to this module's documentation at https://www.drupal.org/docs/contributed-modules/fivestar

🇺🇸United States tr Cascadia

This constant is not present in any of the current versions of Radioactivity.

🇺🇸United States tr Cascadia

While the documentation still needs work, this issue isn't helping that process along - no comments in more than 11 years!

As a reminder, the way documentation is done on drupal.org has changed a lot. The current Radioactivity module documentation guide may be found at https://www.drupal.org/docs/contributed-modules/radioactivity , and ANYONE can edit that or add pages.

So I encourage anyone who is interested in improving the documentation to help out by editing that guide directly.

🇺🇸United States tr Cascadia

None of this is relevant anymore, now that Drupal 7 is no longer supported.

🇺🇸United States tr Cascadia

Drupal 7 is no longer supported, and we don't have these schema problems in the Drupal 8/9/10/11 version of Radioactivity.

🇺🇸United States tr Cascadia

Yes, your MR is good. But it should also declare(strict_types=1) for all these files. We don't need tests for adding type hints.

Part of the problem here is that votingapi_widgets (and other modules) depends on votingapi, and everytime I make a change on votingapi I have to also try to make those same changes in all the dependent modules (I have maintainer privileges only on some of those dependent modules.) When they are all in the -dev stage they are going to be out of sync from time to time. Once votingapi gets a release then we can make a votingapi_widgets release that agrees with that specific votingapi release.

I do have the above changes done locally, but I haven't pushed them yet because I am busy working on a lot of modules trying to bring them all up to standards. I would like get the testing working and improved so that things like this typing issue show up immediately in the tests when votingapi gets changed. So in this case, a simple test of the plugins provided by this module would have flagged this problem, and many other problems that will show up as votingapi gets changed. So while a new test is not needed for the above change, a test of these plugins is really, really needed for long-term maintenance of this module.

🇺🇸United States tr Cascadia

@alexpott: Please clarify the direction you want to take on this one.

I asked "as a way forward, how about we add some common, known formats right now?"
Is this acceptable as an important fix/improvement? Or do we have to continue to suffer with this bug pending a complete examination and re-write of how we do protocol filtering throughout core?

Right now this is going nowhere, because it's not clear where you want to go with this.

I'm happy to re-do the MR along the lines I proposed in #18 if there is some guidance here as to what stands a chance of getting accepted.

🇺🇸United States tr Cascadia

Yes, the text output is not very good - it just adds an HTML element holding the barcode value. While it can be styled to line up with the bar values and aligned with the code itself, that requires a lot of work.

It is definitely something that should be put into this module, but that's a different issue entirely! And will require a good amount of work for a general solution, as this is not something provided by the underlying tecnickcom/tc-lib-barcode library. I honestly don't think many people use/need the text value, since there haven't been any issues opened about this in the past. It would be nice to have some contribution from the community here, from some people who need/want this feature to work better. I personally don't need that, so it's going to be a low priority for me.

So I'm just committing your fix, which will fix the original issue, then I will write up a separate issue for the work that I think should be done on the text output.

🇺🇸United States tr Cascadia

tr created an issue.

🇺🇸United States tr Cascadia

This is a good idea. After thinking about it for a while, I have requested an ownership transfer. See 💬 Taking over the Trending namespace Active

🇺🇸United States tr Cascadia

Contacted current owner via email

🇺🇸United States tr Cascadia

tr created an issue.

🇺🇸United States tr Cascadia

Leaving this open as a reminder that the documentation guide has to be examined and brought up to date.

This is something that ANYONE can do, if you would like to help out.

Production build 0.71.5 2024