🇧🇬Bulgaria @pfrenssen

pfrenssen → created an issue.

This is fixed for 2.x, but can still potentially be backported to the 1.x branch.

Thanks for working on this!

Instead of adding additional code to the form validate handler, let's instead mark the field as required in Registrant::baseFieldDefinitions() as was proposed in the issue summary.

By doing this we will inform Drupal that the field is required. Drupal will then take care of the form validation for us. Also it will do some other helpful things like marking the field as required, support entity validation etc.

We do not need to make any changes to the form, it will all be handled automatically :)

Thanks for the quick response. The attached MR fixes it for me. I am not 100% sure if the patch is omitting certain fields that are necessary for the CSV export since I am not using that feature. Please let me know if anything needs to be done.

pfrenssen → created an issue.

Maybe a better solution would be to remove Drupal 9.5 from the supported versions and require PHP 8. Both Drupal 9.5 and PHP 7.4 are unsupported so the maintainers' efforts are better spent elsewhere.

I made issues for our 4 plugin types:

• 📌 Use attributes to define SchemaExtension plugins Active
• 📌 Use attributes to define DataProducer plugins Active
• 📌 Use attributes to define PersistedQuery plugins Active
• 📌 Use attributes to define Schema plugins Active

Would it be OK to include these in the roadmap for the 5.x branch?

pfrenssen → created an issue.

pfrenssen → created an issue.

pfrenssen → created an issue.

pfrenssen → created an issue.

Now that we are planning the next major, would it be a good idea to get attribute based plugin definitions in? [#SchemaExtension()] rather than @SchemaExtension.

pfrenssen → changed the visibility of the branch 11.x to hidden.

pfrenssen → changed the visibility of the branch 1255092-url-should-return to hidden.

Hiding older MR since it is no longer relevant as per #1255092-66: url() should return / when asked for the URL of the frontpage → . Path alias module is optional.

The automatic merge wasn't triggered.

Fixed PHPStan error. Maybe we can set PHPStan to not allow failures?

Just updating the version strings is not sufficient. I did a check and the following issues also need to be addressed:

1. Class Drupal\mailgun\Commands\MailgunSanitizeCommands implements deprecated interface Drush\Drupal\Commands\sql\SanitizePluginInterface: use \Drush\Commands\sql\sanitize\SanitizePluginInterface instead.
2. MailgunMail.php, line 111: Call to deprecated method renderPlain(). Deprecated in drupal:10.3.0 and is removed from drupal:12.0.0. Use Drupal\Core\Render\RendererInterface::renderInIsolation() instead.

Great news, thanks a lot!

@owenbush do you want to weigh in?

Thanks! Looks good to me. There are some test failures but this is because the module is not yet Drupal 11 compatible, that is unrelated to this issue.

Setting this to major since this is causing a test failure that is affecting all open issues for 3.0.x.

PHPUnit failures are due to 🐛 PHP 8.4: Implicitly marking parameter $parent as nullable is deprecated Active .

Seems like this is blocked on 2 issues :(
Postponing on 📌 RegistrationType enum cases not adhering to coding standards Active and 🐛 PHP 8.4: Implicitly marking parameter $parent as nullable is deprecated Active .

pfrenssen → created an issue.

PHPCS failures are due to 📌 RegistrationType enum cases not adhering to coding standards Active .

pfrenssen → created an issue.

Thanks very much for the offer.

We could really need some help with reviewing the open issues. I was onboarded as a co-maintainer and worked on a number of issues in the second half of 2024. I went through many tickets, reviewed and merged the ones that were ready and started the 3.x branch. Unfortunately the momentum has slowed down a lot because many tickets that are in "needs review" state are the ones that I worked on myself and I couldn't move them forwards.

Moving this to 3.x since this is D11 related.

Thanks for the report, this looks like exactly the kind of issue that the D11 OOP hook system is designed to solve.

It would be good to add #[LegacyHook] to the old hook implementation so it is not executed twice. There is some more information about this in Support for object oriented hook implementations using autowired services → .

pfrenssen → made their first commit to this issue’s fork.

pfrenssen → created an issue.

Thanks very much!

pfrenssen → created an issue.

pfrenssen → made their first commit to this issue’s fork.

I was too quick in deciding to expose the metadata type directly as children of container / composite elements. It is actually better to expose WebformElement because there can be fields on them that are essential for rendering the forms in the frontend.

A good example is the "maxlength" for text fields. This is exposed on WebformElementTextBase which is an interface for WebformElement.

So I reverted ✨ Expose the element metadata directly in nested elements Active again in this MR.

This is looking great, thanks a lot! We can now configure PHPStan to not allow any failures and this is good to go.

I agree, Drupal 10.2 is no longer supported so we can drop it and set Drupal 10.3 as the minimum version.

I have done the first part of the work: I added support for generic composite elements. It looks like there are a few gotchas with composite elements:

The webform module expects the values for the child elements to be passed as parent_element_key[0][child_element_key] but then the validation messages are returned as parent_element_key[items][0][child_element_key]. This is going to be confusing for the GraphQL consumer that there is an unexpected [items] part appearing in the element key.

Another thing I noticed is that when a child element is required, and the composite element is configured to show N number of empty rows, that the validation errors returned by the Webform module are duplicated N times. I am guessing this is intended to make it easier to highlight the validation error in the Drupal rendered webform for the N empty rows. But for the GraphQL consumer this might be confusing since they might get more validation errors back than values they are submitting.

A third thing is that, since we don't know in advance how many child elements are going to be rendered for a composite field that allows multivalue child elements, the Webform module returns element keys like parent_element_key__child_element_key. This will need to be converted into parent_element_key[N][child_element_key] by the frontend implementation. We need to document this somehow.

I think in scope of this issue I am not going to try to transform the internally used logic for handling validation of these elements and just return what we get. Now we have some special cases when handling validation of composite elements, but there will probably be more edge cases in other elements. If this becomes problematic in the future we can implement a plugin system to handle these kinds of edge cases for each Webform element type.

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 like Edit 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).