Account created on 24 February 2012, over 13 years ago
#

Merge Requests

More

Recent comments

🇺🇸United States jcandan

Actually, I just thought of a use-case for us where this would come in handy. And, not only does that #states ticket get us part of the way there, 🐛 content_moderation support Needs work introduces content moderation published states as well. And, yes, from there, I agree, some logic and a UI to handle state selection would get us the rest of the way. Looking forward to this.

Postponed until the above are adopted.

🇺🇸United States jcandan

Rolls fresh patch from MR !27.

🇺🇸United States jcandan

jcandan changed the visibility of the branch 3178100--content-moderation to hidden.

🇺🇸United States jcandan

jcandan changed the visibility of the branch 8.x-1.x to hidden.

🇺🇸United States jcandan

Original summary

Problem

The logic that has been added to the RequireOnPublishValidator for the paragraph entities is not correct. Paragraph can't get the parent entity since it doesn't exist when a user creates a new node. Also, even on the node edit, when a node exists and the paragraph can extract the parent entity, it will get the original node rather than a new one and therefore it will contain the old 'Published' value.

  • Create a paragraph type with the 'Require on publish' option enabled.
  • Add this paragraph to a node.
  • Create a new entity with an unchecked 'Publish' option.
  • Observe that 'Require on publish' validator returns an error for the required paragraph fields.

Proposed resolution

  1. Since at the moment, when a user submits the form validation constraint doesn't have the fresh node, we need or extract the published value from the POST data
  2. Skip the validation for the paragraph entities and when constraint function goes through all entity fields, find paragraphs fields, get paragraph entity and then go through all paragraph fields as well.
🇺🇸United States jcandan

Okay. There is quite a bit going on here.

  1. The original issue summary steps to reproduce make no mention of content moderation.
  2. It turns out that he did fix the original issue as stated in #6.
  3. Others jumped in and suggested it was content moderation where it failed, which is true after #6 was merged.
  4. From thence, many have put forward several independent efforts to address this issue.
  5. Including my own duplicate issue noted above.

I will mark my issue as a duplicate, as well as 💬 Paragraphs integration not working with workflow moderation Active .
I will port my work from 🐛 Fix Paragraphs support with Content Moderation Active so I can give folks here credit for their contributions.

Once I get that ported over to a new MR associated with a fork here, please conduct reviews to ensure this addresses the issue well.

🇺🇸United States jcandan

By the way, just to clarify why I think Add require on publish as a conditional field option Active may be related is that it introduces state handling for conditions (e.g. when some other field is check, make this one required on publish).

Though it may seem 🐛 Fix Paragraphs support with Content Moderation Active has more to do with this module's Paragraphs support, it adds support for moderation states that are considered Published.

I don't think we had this before.

            $workflow = $this->moderationInfo->getWorkflowForEntity($parent);
            $new_state = $workflow?->getTypePlugin()->getState($moderation_state[0]['state']);
            if ($new_state) {
              $is_published = $new_state->isPublishedState();
            }

I don't think that fully encompasses this issue's desired outcome, but it may be a step in the right direction.

🇺🇸United States jcandan

Do you still want to support Drupal 9? This might be worth making the breaking change for 🌱 Require on Publish 8.x-1.11 or 2.0.0 stable release plan Active .

🇺🇸United States jcandan

Ya know! Actually, I missed this ticket when I created 🐛 Fix Paragraphs support with Content Moderation Active . Please take a moment to review the patch/MR there. Sorry if this ended up being a duplicate ticket!

🇺🇸United States jcandan

Rolled MR !24 into a stable patch file.

🇺🇸United States jcandan

Ya know! Actually, I missed this ticket when I created 🐛 Fix Paragraphs support with Content Moderation Active . Please take a moment to review the patch/MR there. Sorry for the duplicate effort!

🇺🇸United States jcandan

My bad, I missed this ticket when I created 🐛 Fix Paragraphs support with Content Moderation Active . Please take a moment to review the patch/MR there. Cheers!

🇺🇸United States jcandan

Obviously, if any tickets make the list that introduce breaking changes, going with 2.0.0 is appropriate. However, given the broad community adoption of the un-prefixed, semantic version, it may be prudent to use this opportunity to make the switch now to 2.0.0, even if no breaking changes are introduced. Just something to consider.

🇺🇸United States jcandan

Title scope seems to duplicate Required on Workflow State Active , however a cursory review of the issue summary and discussion lead me to think not so.

Either way, I do wonder if there's any chance Add require on publish as a conditional field option Active fixes this. I haven't tested, but since it adopts #states condition handling, that might address this issue.

🇺🇸United States jcandan

Any chance Add require on publish as a conditional field option Active fixes this? I haven't tested, but since it adopts #states condition handling, that might address this issue.

🇺🇸United States jcandan

As per #13, updating the scope of this ticket to target the name field only. Address field already has a ticket in the queue: 📌 Add support for Address field Active .

🇺🇸United States jcandan

Updated the issue summary based on understanding captured in Add support for multi-value fields like Name and Address fields Active .

Original report

Problem/Motivation

Address field is actually detected by 'Require on publish' module ('Warning on Empty' message is well displayed) but no indicator is added to the field label.

Might be useful for all other fields that output their widget #type as 'details'.

Remaining tasks

Add the missing 'form-required-on-publish' class to the <summary> tag.

🇺🇸United States jcandan

If 🐛 Fix tests failing due to unknown option verbose Active gets adopted, you'll need to re-opt-in for Drupal 11 testing.

🇺🇸United States jcandan

By adding full support for #states Form API conditions, integration with Conditional Fields is possible with:

use Drupal\Core\Form\FormStateInterface;

/**
 * Implements hook_conditionalFieldsStates_alter().
 *
 * @see Drupal\conditional_fields\Conditions::conditionalFieldsState
 * @phpcs:disable Drupal.NamingConventions.ValidFunctionName.InvalidName
 */
function require_on_publish_conditional_conditionalFieldsStates_alter(array &$states) {
  // phpcs:enable
  // A list of supported states that may be applied to a conditional field.
  $states['require_on_publish'] = t('Required on Publish');
}
🇺🇸United States jcandan

Fork description

So that the module doesn't depend on Conditional Fields, the logic is provided in a submodule. When enabled, it provides require_on_publish as one of the possible #states.

Steps to reproduce

Using the provided fork, you may begin to understand how this might work.

  1. Install Require on Publish and Conditional Fields.
  2. Add a boolean field to Basic Page, and a text field.
  3. Under Structure > Conditional Fields, make the text field required on publish when the boolean field is checked.
  4. Create a Basic Page.

With the above and the branch as it stands, the required on publish markings are successfully applied. However, the error handling still needs work.

Problems persist

See comment above and comments in code for an understanding of the ongoing problems.

🇺🇸United States jcandan

I've pushed a couple of commits to the fork (noting here in case someone overrides):

I am still working through some issues:

  1. I think I need to prevent error message removal when the condition is not met in \Drupal\conditional_fields\ConditionalFieldsFormHelper::formValidate().
  2. It seems the dependentValidate() thinks the condition is "triggered" when it is not.
  3. The dependentValidate() also seems to be explicitly handling required and !required.
  4. Not yet sure if it is best to pass the _wrapper in the error key.
🇺🇸United States jcandan

@Katy Jockelson , I think I have good news. I was able to reproduce the issue. Did you have content moderation enabled for your content type with the RoP Paragraphs field? If so, it is being skipped outright.

I have reported it as a bug: 🐛 Fix Paragraphs support with Content Moderation Active . Follow there. Cheers!

🇺🇸United States jcandan

Fixed issue where other field validation messages were being overridden.

🇺🇸United States jcandan

When a simple field is required on publish, and the compound (e.g. name) field error validation is ordered before the simple field's messages, the messages are all replaced by the @field_label also requires the following parts when publishing: <em>@components</em>.

🇺🇸United States jcandan

Needs work: Incorrectly replacing other field validation error messages with @field_label also requires the following parts when publishing: <em>@components</em>.

🇺🇸United States jcandan

Patch #4 successfully applied and resolved the issue.

🇺🇸United States jcandan

@Katy Jockelson , I am unable to reproduce the issue as described. I tried a vanilla Drupal instance. Created a simple Paragraph type with one field required on publish and the other not. I added that Paragraph field to Basic Page. The markings and validation worked as expected. Drupal 10.4.3. Require on Publish 8.x-1.10.

🇺🇸United States jcandan

Even though features like embedding timelines are working as expected here https://publish.twitter.com/ with the same principle.

Seems to no longer be the case:

🇺🇸United States jcandan

This makes sense. The ordered list number 4 needs to be updated to 3.

🇺🇸United States jcandan

@plopesc, thanks for your work on this.

Your test correctly failed prior to the fix, and passes after the fix. I've refactored a bit, removing the AJAX adjustments and adjusted the conditional logic for persisting the country selection. Feel free to add an issue if you'd like to describe steps to reproduce the AJAX issue your team is experiencing.

MR !14 is ready for review. Will leave this open for a couple days to receive feedback, but this looks good to me.

🇺🇸United States jcandan

I'd imagine that, until this is added to Radix, in order to adopt Storybook , we'd just have to create our own *.stories.twig files in our sub-theme, and for those, a *.compentent.yml with the replaces key pointing to the target component in Radix. Anything else I may be missing?

🇺🇸United States jcandan

Added Bump supported intl-tel-input version Active . Seems appropriate to get the front-end package dependency a good solid update before a major release.

When 2.0.0-rc2 gets released, it will introduce breaking changes (changes that require consuming applications to make code modifications to continue working correctly with the new version). While release candidates may be considered close to stable, this update is long overdue, and introducing breaking changes on a release candidate isn't strictly forbidden. We'll be sure to communicate this in the release documentation for RC2.

🇺🇸United States jcandan

@plopesc pointed out that this feature may already be addressed by Telephone Validation . I wonder if @omkar-pd might be willing to share reprucible steps if this becomes an issue with the modules not playing nicely.

🇺🇸United States jcandan

Removed 🐛 Fix validation not being applied Active as a release blocker. Willing to consider putting it back in. Awaiting community feedback.

🇺🇸United States jcandan

I realized that this should be marked as a feature request, not a bug. To fail validation and prevent save might even be considered a breaking change. Though, if the community wants this, we could add it to a release candidate before making it to 2.0.0. Perhaps even make it configurable.

Moving this out of 📌 Release 2.0.0 Needs review , unless someone objects.

🇺🇸United States jcandan

In your video, you type the exit code and country code into the field, rather than select the flag. I was able to reproduce this bug by selecting the flag.

🇺🇸United States jcandan

I was able to reproduce this issue. Digging.

🇺🇸United States jcandan

Need to verify this indeed has been fixed.

I created this ticket based on #3302638-3: Telephone International Widget 2.0.0 stable release plan . I assumed the front-end validation from 🐛 Doesn't work with multiple phone fields Needs work was sufficient, but I realize now that this is referring to back-end validation.

🇺🇸United States jcandan

Absolutely! Thanks for calling that out, @johnpicozzi. I'll take a moment to look through the other tickets which may have influenced the total sum of changes captured here. Thanks everyone for your patches, MRs, and review.

🇺🇸United States jcandan

As part of the work done in 🐛 Doesn't work with multiple phone fields Needs work , we load the utils.js as part of the *.libraries.yml.

🇺🇸United States jcandan

The composer.libraries.json has been removed as part of 🐛 Doesn't work with multiple phone fields Needs work . See that ticket for details.

🇺🇸United States jcandan

Got a test added and refactored the work from the above patches and MR !8. Good job and thanks , everyone.

Please, someone review MR !9.

🇺🇸United States jcandan

Never mind. That intlTelInputWithUtils.js doesn't even come with the package. Must be outdated docs.

🇺🇸United States jcandan

Option 1: intlTelInputWithUtils
If you're not concerned about filesize (e.g. you're lazy loading this script), the easiest thing to do is to just use the full bundle (/build/js/intlTelInputWithUtils.js), which comes with the utils script included. This script can be used exactly like the main intlTelInput.js - so it can either be loaded directly onto the page (which defines window.intlTelInput like usual), or it can be imported like so: import intlTelInput from "intl-tel-input/intlTelInputWithUtils".
-- https://github.com/jackocnr/intl-tel-input?tab=readme-ov-file#loading-th...

Given this is loaded when the widget is used, let's adopt the above implementation in lieu of the changes made from #11.

🇺🇸United States jcandan

Merged 📌 Remove the composer libraries workaround Active . This ticket can be updated to reflect any desired changes.

Production build 0.71.5 2024