pfrenssen → made their first commit to this issue’s fork.
The version range is too broad, we should only declare compatibility with versions that have been tested and are known to work. Using open ended version ranges will allow the module to be installed on any future version and that will cause sites to break.
Reviewed the patch, it looks good. Only the info file needs to be updated, no other necessary changes are detected by Upgrade Status.
pfrenssen → changed the visibility of the branch 3430062-automated-drupal-11 to hidden.
The TransactionDataEvent has been deprecated in favor of the PaymentIntentEvent but both events seem to function in different ways. There is no direct upgrade path between the methods in both events.
It would be great if we could have a change record with before/after examples that showcase how to migrate from the old event to the new event.
Also, can we have some information on what this is postponed on?
pfrenssen → changed the visibility of the branch 3431472-automated-drupal-11 to hidden.
pfrenssen → changed the visibility of the branch project-update-bot-only to hidden.
pfrenssen → changed the visibility of the branch 3363802-drupal-10-comatability to hidden.
Deleting the cache files is OK for non-persisted bins but not for persisted bins. We should move them to the new folders or the persisted data will no longer be available to Drupal after updating the module.
Applied the patch from #5 and removed an unneeded change. The fix works to prevent the error from the issue summary. I reviewed the patch and it looks good. It is OK to set an empty value on the new field, because the module can handle a missing value. There is a check for it in BehaviorSettingsManager::getEntityBehaviorSettings().
There are multiple different errors being confused here. Let's focus this only on the error mentioned in the issue summary. Comments #2 #6 #8 #10 are different error messages and are not in scope of this issue.
I like the idea of using an md5 hash of the cid, this will more evenly spread out the cache files over many different folders. I am wondering if we should provide an update path to move the existing files into the folder for cache bins that have the persisted option set?
Great that you have been able to work around the problem. I will for now close this issue as fixed. Some more ideas:
- To save on disk space, you could try to employ compression, see ✨ Serializer should be configurable Fixed .
- Make sure cron is running, the system cron job will trigger garbage collection on cache bins, which should delete invalidated cache entries.
- Using file cache for all the cache bins is probably not the best idea. You can decide for each cache bin which cache backend is the most suitable. For example you can store session data in Redis, page cache on the file system, render cache in the database, etc.
I have been going through the settings and while there are hundreds of settings, I think the vast majority are not useful at all for a decoupled frontend. Probably some settings can be useful in some rare cases, but most of them are intended for administration purposes or to control backend processes.
I think the page URLs are probably the ones that are most useful, then a site builder can choose on which path the webform is shown in the frontend. The messages for open / closed forms are also useful, but we don't yet expose this status, so I split this off to a separate issue #3552668: Report whether a form is open or closed → . The rest of the settings seem rather useless for the majority of decoupled frontends.
If a setting that you need is not exposed, feel free to open a feature request!
I reviewed the MR but it looks like the problem was already fixed. I checked the git log and indeed it was fixed recently in 🐛 JavaScript error in toolbar.js when using paragraphs_edit frontend editing Active . Marking as a duplicate.
Since this is also affecting older versions of Drupal, it could be interesting to backport this to 11.2.x and 10.x. See for example 🐛 document.querySelector is null Active which reports this issue affecting 10.1.x.
It is totally OK to use AI agents to help with repetitive tasks like adding missing documentation, but please review every single line of AI generated code carefully before committing it. The LLMs can make a right mess of things. I just spent my entire evening fixing hundreds of lines by hand.
Anyway apart from this, the sheer amount of work that was done is incredible, thanks a lot @jeremy1606 for the effort!
This still needs another review before it can be committed.
Cherry-picked the fix into the MR for 📌 Automated Drupal 11 compatibility fixes for currency Needs review . Closing as duplicate.
I did a review of the MR. There is still some work left.
This is a valid problem, but since this is Drupal 11 specific, it would be better to roll this into 📌 Automated Drupal 11 compatibility fixes for currency Needs review .
pfrenssen → made their first commit to this issue’s fork.
I did an iteration on it, but there is still an issue with this. It looks like there is a difference between [node:field_link_2:path] and [node:field_link_2:url:path]:
[node:field_link_2:path]seems to return the relative path including the base URL (/web/foo/bar).[node:field_link_2:url:path]seems to return the relative path without the base URL (/foo/bar).
This is interesting and yes I agree 255 characters will not be enough for all possible use cases.
I found a discussion about URL lengths on Stack Overflow: https://stackoverflow.com/questions/417142/what-is-the-maximum-length-of...
In short, it highly depends on the use case. In general there is some consensus in this discussion on using 2048 characters as a limit, but that's possibly outdated because this seems to have been mainly a limit of Internet Explorer. Some clients apparently can handle URLs up to 2GB, but that would be a tad excessive.
Needs to be rerolled now that 📌 Convert hooks to OOP Active is in.
Hiding patches, drupal.org is moving to merge requests. See Using Gitlab to contribute to Drupal → .
pfrenssen → changed the visibility of the branch 2995680-error-on-call to hidden.
Created new MR against 2.0.x. It looks like 2.1.x has not been worked on for a long time, and 2.0.x is more recent. I can rebase against 2.1.x if needed.
#11 is right, I received a bug report with the following error logged:
Error: Call to a member function first() on null in Drupal\address\Plugin\views\field\Subdivision->render() (line 91 of modules/contrib/address/src/Plugin/views/field/Subdivision.php).
Let's also account for that. I will update the MR.
This issue is also affecting contrib, e.g. Entity Reference Revisions: #3549453: Exception when cancelling user account → .
I did some very limited testing, but it looks like propagating the syncing state from the host entity to the referenced entity is enough to prevent the exception from being thrown.
In any case this is postponed until 🐛 Revision user incorrectly appears as anonymous user when node author is cancelled Needs work is in.
Addressed #127 and the remarks on the MR. Extended the test coverage to include a forward draft revision, which covers the change in ContentEntityBase.
Updated code to work with latest 11.x. There were conflicts due to 📌 Remove NodeHooks1 Active and 📌 Create a new NodeRepository for the code from Drupal\node\NodeStorage Active .
None of the remarks have been addressed. Now that 📌 Create a new NodeRepository for the code from Drupal\node\NodeStorage Active is in it is expected that there will be deprecation warnings.
pfrenssen → made their first commit to this issue’s fork.
The Webform module stores the default values in YAML format, so they are structured data. We can expose it as JSON encoded strings so the GraphQL API consumer can figure out what to do with it.
Indeed some types are not great, but passing what is stored is already a good start.
I couldn't find any token replacement happening with the default values. But I might have overlooked it.
Awesome improvements, thanks a lot for the contribution! Looking good to me, will merge when tests are green.
Thanks for this! A lot to unpack, assigning to me for review.
pfrenssen → made their first commit to this issue’s fork.
Very nice work! I removed a duplicate exposure of the `alignItems` property. This is ready to be merged. Thanks a lot for the contribution!
I aligned both competing branches, this is ready to go. Thanks a lot for the contribution!
Hey I saw you are working on this, but I am already done with the changes. I'm sorry it was not clear, I had assigned the issue to me to indicate I am taking care of it.
Thanks a lot for working on this! I have reviewed the code, it's looking great so far. I left some comments on the MR. We also need a test. I can take care of that.
Agreed, this simplifies the API because it is not so developer friendly having to deal with a data type just with one optional string in it. Thanks for the contribution!
Thanks for starting this!
We should have a closer look at these and they value they bring for building decoupled forms in the frontend. Probably many of these are very useful and will expose more of the webform element settings to the frontend.
I think some of the options that are in the "General" tab are very useful: especially the "Disabled" option is great, as well as the field prefix and suffix.
Also exposing the help text/title, and the read more text/title are useful but we will need to document this clearly in the schema so the frontend dev can understand what are the differences between the description, the help text and the more text.
Regarding the options to control the positioning of the title, description and help text. All of this is used to control the look of a webform when rendered inside a Drupal theme, but I think it is not very likely that a decoupled frontend will implement these in 4-5 different positions? But then again there is not harm in exposing this information. Since these are discrete options it would be more in line with the GraphQL philosophy if they are represented as enums.
Some options are specific to the backend and don't seem to make sense to expose to the frontend. For example "unique" will trigger a check that no duplicate values have been entered in the database. The frontend cannot validate this on their end so they cannot do anything with this information.
I had a look at the options under the Advanced tab, but these seem to be quite specific to how Drupal renders form fields. For example the custom attributes and wrapper CSS classes are used in the render arrays and Twig templates of the Field API, and don't seem to be too useful in the context of a decoupled frontend. Maybe we can hold off on these unless people really need them.
Also I would like to avoid putting serialized objects in the data. This is not going to be super helpful for frontend devs who don't typically have a PHP background ;)
This change does not seem to be needed. The current implementation already covers it. The WebformRequiredElement is only present when the element is required. When it is NULL then the element is not required.
With this MR, we will pass a message for a required element if the element is not required, that can be confusing.