Thanks so much for coming up with this smart workaround!
This is a great suggestion. The help text configuration in the Webform module consists of 3 configuration options which can be exposed as metadata fields:
metadata {
help: String,
helpTitle: String,
helpDisplay: WebformHelpDisplayOption
}
enum WebformHelpDisplayOption {
ELEMENT_AFTER
ELEMENT_BEFORE
TITLE_AFTER
TITLE_BEFORE
}
I split off the removal of the nesting level to ✨ Expose the element metadata directly in nested elements Active . I will implement that first because it will simplify this issue.
pfrenssen → created an issue.
I started on this. It looks like we can also support the fallback element named WebformElementWebformAddress
which works out of the box (without needing the Address module).
The address elements are so-called "composite" elements which share a lot of similarities to other elements like "Flexbox" and "Fieldset". We can unify these under a shared WebformElementContainerBase
interface.
Now that
✨
Move properties on the WebformElement interface
Active
is in we can remove 1 level of nesting by directly exposing WebformElementMetadata
on the child elements
rather than exposing WebformElement
which has become just a wrapper intended to slim down the schema.
Thanks!
pfrenssen → created an issue.
pfrenssen → created an issue.
This is looking great, thanks very much!
pfrenssen → created an issue.
pfrenssen → created an issue. See original summary → .
pfrenssen → created an issue.
Thanks for the feedback @dulnan! I will implement this.
Unassigning, let's first figure out the best way to approach this.
The GraphQL specification actually required fields defined on an interface to be duplicated across all the types that implement the interface.
I mistakenly thought we could generate a schema like this:
interface WebformElement {
key: String!
title: String
description: String
multiple: Int
required: WebformRequiredElement
type: String!
}
interface WebformElementMultipleValuesBase {
multipleValues: WebformElementMultipleValues
}
type WebformElementAddress implements WebformElement & WebformElementMultipleValuesBase {
}
type WebformElementCaptcha implements WebformElement {
}
But the GraphQL interface specification actually _requires_ the fields to be duplicated on the implementors, with the motivation that this makes it easier to declare covariant types.
So it quickly becomes bloated like this:
interface WebformElement {
key: String!
title: String
description: String
multiple: Int
required: WebformRequiredElement
type: String!
}
interface WebformElementMultipleValuesBase {
multipleValues: WebformElementMultipleValues
}
type WebformElementAddress implements WebformElement & WebformElementMultipleValuesBase {
key: String!
title: String
description: String
multiple: Int
required: WebformRequiredElement
type: String!
multipleValues: WebformElementMultipleValues
}
type WebformElementCaptcha implements WebformElement {
key: String!
title: String
description: String
multiple: Int
required: WebformRequiredElement
type: String!
}
Since we are still building out the schema it is easy to see how this can bloat the schema exponentially.
A good solution to limit this could be to define only a single element on an interface, for example:
interface WebformElement {
metadata: WebformElementMetadata!
}
interface WebformElementMultipleValuesBase {
multipleValues: WebformElementMultipleValues
}
type WebformElementMetadata {
key: String!
title: String
description: String
multiple: Int
required: WebformRequiredElement
type: String!
}
type WebformElementAddress implements WebformElement & WebformElementMultipleValuesBase {
metadata: WebformElementMetadata!
multipleValues: WebformElementMultipleValues
}
type WebformElementCaptcha implements WebformElement {
metadata: WebformElementMetadata!
type: String!
}
Which is incidentally how it was in version 2.x-alpha1, but this got flattened out as part of the change request ✨ Remove Metadata subfields Fixed .
It is probably a good idea to revisit this, and to reconsider adding back the metadata
field to limit schema bloat.
We should also take other measures to limit the size of the schema. For example we should not expose all field types by default.
Starting on this.
Thanks @agunjan085 for picking this up! Just a heads up, this is an issue intended for the 2.x-dev branch, but the merge requests are still made for the old 8.x-1.x branch. Please make sure you are working on the right branch!
Unfortunately I do not have the right permissions to fix the default branch in Gitlab.
pfrenssen → created an issue.
pfrenssen → created an issue.
Thanks for the updates @VladimirAus, I am really happy with how this is shaping up. Not only is the PHP error solved in a clean way, but we also have a helpful exception that can be caught by calling code and gives useful information about the underlying problem.
I would like to remove the second test because I think it is unnecessary and would make future maintenance of the class harder. And we have a PHPStan warning. I left comments on the MR.
pfrenssen → created an issue.
pfrenssen → created an issue.
I've hidden the outdated branches, the branch from @guiu.rocafort.ferrer is the current focus :) Thanks a lot working on this!
pfrenssen → changed the visibility of the branch 11.x to hidden.
pfrenssen → changed the visibility of the branch 3301239-postreamreaderreadline-throws-an to hidden.
pfrenssen → created an issue.
A Drupal 11 compatible release has been made: https://www.drupal.org/project/graphql_webform/releases/2.0.0-alpha2 →
Duplicate of 🐛 Use of unsupported exclamation mark placeholders Active .
pfrenssen → created an issue.
pfrenssen → created an issue.
pfrenssen → created an issue.
As far as I can tell, there's no error handling here if the $rec->paren_type is none of the above cases, right? Maybe logging/throwing an error here would be an appropriate error handling.
The module doesn't seem to query any other entity types at the moment (ref. ::getSqlLeftJoin()) but I will add a fallback to future proof it. Logging or throwing an error message would not be useful IMO because this would alert the site owner but they cannot do anything on their end to solve this.
I believe the
Paragraph @id
naming doesn't have enough information value, as it's very similar to the first entry of the row and doesn't convey the information that this is an edit link. Therefore I suggest keeping the@id/edit
naming or changing it to something likeEdit Paragraph @id
.
Good suggestion. It is confusing that the header says "Edit link" while the links no longer lead to edit links but instead to usage reports. I will fix this.
If I read the code correctly, the $element variable never gets used except at the return of the function.
That is intentional, $element
should be returned, changed or unchanged. Here is an example in the API docs that shows this pattern: EntityForm::afterBuild().
The error is originating from migrate_drupal_migration_plugins_alter()
but should not be emitted. The code is doing a requirements check for migrating the "variables" subsystem, but this doesn't exist in Drupal 8+ since variables were replaced with config.
There is similar code just above that will suppress the exception for sites running Drupal 7+ which do not meet the requirements of the D6 taxonomy vocabulary migration:
catch (RequirementsException $e) {
// This code currently runs whenever the definitions are being loaded and
// if you have a Drupal 7 source site then the requirements will not be
// met for the d6_taxonomy_vocabulary migration.
}
We can follow the same pattern, something like this:
catch (RequirementsException $e) {
// This code currently runs whenever the definitions are being loaded and
// if you have a Drupal 8+ source site then the requirements will not be
// met for the system_site migration since the 'variables' subsystem was
// replaced with config entities in Drupal 8.
}
Idea: allow to set a revision log message and user ID.
Thanks for the review! I agree that it looks suspicious that we are changing the form state in this callback, but that's a supported way to set values in the form state during form processing.
To put your mind at ease here is an example from Drupal core that shows the use of the #after_build
callback to alter the form state in core. It is determining if CKEditor should be the activated on the page: assessActiveTextEditorAfterBuild.
The Form API has been designed almost 20 years ago (introduced in Drupal 4.7) and it was already revolutionary to use an object for the form state rather than an associative array :) There are some long-term plans to refactor the Form API (which is a part of the Render API) to become object oriented, but since the scope of this task is insanely huge there is nobody actually working on it (ref. #2821399: [meta] Theme System Modernization Initiative → ).
pfrenssen → created an issue.
Solved the merge conflict.
The patch in #107 is an export of the MR for people who prefer to use a fixed patch rather than the moving head of an MR. It should work against the latest 4.2.0 release of the module on supported versions of Drupal (10.3 and up).
pfrenssen → changed the visibility of the branch 3499738-adhere-to-coding to hidden.
Tests are passing on:
- Drupal 10.4.1 running on PHP 8.1.31
- Drupal 11.1.1 running on PHP 8.3.15
- Drupal 11.1.1 running on PHP 8.4.2
I started on this but there are a large number of violations. It would be best to handle this in a series of small focused MRs.
I really like this extended help text, this is going to be super helpful for the event managers.
pfrenssen → created an issue.
pfrenssen → created an issue.
pfrenssen → created an issue.
Thanks for the reminder!
The MR looks good to me, this is just a trivial version string update.
pfrenssen → created an issue.
pfrenssen → created an issue.
pfrenssen → created an issue.
The DDEV command to regenerate the Nginx config is very useful! Thanks for adding that!
I really like the improvements to the README but I would not add the additional scripts. The scripts are super cool but they don't really fit in the scope of a starterkit. They would fit better in a BLT-like project intended for mature websites which are already in production. Also I think most developers will have their own collection of scripts tailored to their own preferences.
In case you have been using an older version of the patch, you'll need to update the plugin ID of the file upload REST resource, since the latest versions of the patch have switched from the webform_rest_file_upload
to webform_rest:file_upload
to follow the convention used in the Attributes based plugin definition in Drupal 10.3.
You can use an update hook to fix this, something similar to this should do the trick:
/**
* Update the plugin name of the Webform REST file upload resource.
*/
function mymodule_update_10101() {
// In recent versions of the patch we are using to add file upload capability
// to the Webform REST resource, the plugin ID was changed.
// @see https://www.drupal.org/project/webform_rest/issues/2899902
$config = \Drupal::configFactory()->getEditable('rest.resource.webform_rest_file_upload');
$config->set('plugin_id', 'webform_rest:file_upload');
$config->save();
}
This is in the meantime outdated, there has been a full release out for a long time. Closing.
I have backported the changes from @markwittens and updated the code to be conform the latest changes in Drupal 10.3+. Fixed tests and cleaned up.
As far as I can see everything discussed in the last bunch of comments has been addressed. I will also upload a patch with the latest state of the MR for the patch lovers.
What is not included is the support for filenames with special characters which was discussed in
#2899902-94: Support file uploads →
-
#2899902-95: Support file uploads →
-
#2899902-96: Support file uploads →
. I noticed that the FileUploadResource
from the core File module doesn't do anything special to handle special characters, which prompted me to do some further research.
- It was suggested in comment #94 to URL encode the filename, but this is not supported by the W3C standard (ref. https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Dispos...). Probably there was some confusion between the
filename
andfilename*
parameters. Thefilename
parameter we use should be quoted, not URL encoded. - Drupal core already offers developers a way to handle filename sanitization: the
FileUploadSanitizeNameEvent
event listener. The idea behind this is that uploaded files are sanitized consistently across the whole site, no matter which module is handling the uploads. - There are already multiple modules that offer filename sanitization using the above event. Examples are Transliterate Filenames → and Filename Transliteration → .
- Drupal 11 has native support for handling special characters, this is amazing! It has been added in ✨ Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc) Fixed
So in conclusion we should not handle filename sanitizing ourselves but let dedicated modules handle it. Drupal 11 has it built in, while users of Drupal 10 and below can use the Transliterate Filenames or Filename Transliteration modules.
In the meantime Webform REST 4.2.0 → has been released and it already requires Drupal 10.3+ as a minimum, so this is not controversial any more :) The old release 4.1.0 still supports older versions of Drupal.
We also have test coverage again which is great!
pfrenssen → changed the visibility of the branch 2899902-sendupload-files to hidden.
In Drupal 10.3 the FileUploadResource
class has been completely refactored (ref. [#3402032]). The new code is much cleaner and leverages a FileUploadHandler
which abstracts away much of the complexity of uploading files.
Unfortunately this change means that we will lose compatibility with Drupal 10.2 and below. This might be controversial since we are at the moment still compatible even with Drupal 8.7.
I personally think we should go ahead with this since Drupal 10.2 and below are unsupported. I will make a new MR though so that projects on older versions of Drupal can still use MR #9.
Makes sense!
Since we are adding Gitlab CI integration now, would it be a good idea to first merge ✨ Switch to Gitlab CI Needs review which goes a step further and also has all the secondary jobs passing (such as PHP CodeSniffer and CSpell)?
Very nice work, looks great!
I'm raising priority to major since we are without test coverage right now and this is basically blocking any open issues from being merged.
pfrenssen → changed the visibility of the branch 2899902-drupal-10-3-1-renamefile to hidden.
Please add proposed changes to the MRs instead of posting them in patches, it is easier to review commits in MRs. It is counterproductive to download patches and comb through them line by line to discover which lines have changed.
It looks like something went wrong with MR #20. It contains ~20000 lines of unrelated code. I propose we close that one and backport the Drupal 10.3 fixes into the original MR.
pfrenssen → created an issue.
I can confirm the patch fixes the reported error. There are some other queries that also need to be updated to be compatible with only_full_group_by
. Let's fix those also in scope of this issue.
Example:
Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42000]: Syntax error or access violation: 1055 Expression #2 of SELECT list is not in GROUP BY clause and contains nonaggregated column 't.entity_type' which is not functionally dependent on columns in GROUP BY clause; this is incompatible with sql_mode=only_full_group_by
xurizaemon → credited pfrenssen → .
pfrenssen → created an issue.
pfrenssen → created an issue.
pfrenssen → created an issue.
Added test.
We should also sort by priority in \Drupal\graphql\Plugin\SchemaExtensionPluginManager::getExtensions()
which is used by non-configurable schema plugins.
Maybe a warning is most appropriate. If the index has been configured to use immediate indexing, then the site administrator expects this to happen and a warning should be issued if it didn't for some reason.