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

Merge Requests

More

Recent comments

🇺🇸United States jcandan

Added tests. Tests pass, but since the PHPUnit update bug they fail.

Needs review.

🇺🇸United States jcandan

This may prove more complex than originally thought. See #3126210-3: Address with default country code always validates as required .

I can remove the HTML5 required attribute via any number of hooks, but backend validation still applies and the fields are required on submit (published or not).

I'm glad we separated this from Add support for multi-value fields like Name and Address fields Active . In the meantime, as a not-so-great workaround, those wishing to apply Require on Publish to an Address field may consider leaving off a default value, and assuming the user's intent to fill out the full address when they provide a country.

🇺🇸United States jcandan

Moving forward with the release. 🐛 content_moderation support Needs work will come in a patch shortly thereafter.

🇺🇸United States jcandan

Released 2.0.0-rc2!

Very happy with the tests added in 🐛 content_moderation support Needs work . Very strong candidate for 2.0.0. That ticket needs review.

🇺🇸United States jcandan

Added significant test coverage including Paragraphs and Content Moderation support. Needs review. Any takers?

🇺🇸United States jcandan

Needs test coverage.

🇺🇸United States jcandan

Bumps previous phpunit job to this phpunit job from 8 to 9 tests and 73 to 77 assertions. Happy with the result.

🇺🇸United States jcandan

Given the jump in assertions, I consider this a success.

🇺🇸United States jcandan

Never mind. This only becomes a problem when we introduce #states Form API handling. Providing the third-party setting programmatically is the correct path forward for tests in Add support for multi-value fields like Name and Address fields Active . Postponed until we work on Add require on publish as a conditional field option Active .

🇺🇸United States jcandan

@anirudhsingh19, thank you for contributing! We're close. We need test coverage. I got us part of the way there by adding Name as a part of the CI pipeline.

However, I recognized a problem while writing tests for this. We're currently blocked by 📌 Ensure require_on_publish setting is applied when FieldConfig is created programmatically Active .

🇺🇸United States jcandan

My bad--I didn't explain why I'd switched the version. It seems 2.x-dev should be deleted while a minor dev releases exists. See 🐛 Fix Composer stable, caret constraint installs dev release Active .

@jeroen dost, I agree this affects both, but it seems--as with many other issues-- 📌 Refactor custom JS for CKEditor5 v45+ Active will address this issue. I wonder if we need to ensure the scope of that ticket includes this, or just mark this as a 2.x-dev issue.

I can see an argument for either way. Just having this note here may be sufficient. :)

🇺🇸United States jcandan

Also, in addition to the problems described, it seems 2.x has been abandoned anyway (source).

🇺🇸United States jcandan

Pipelines fail. Needs fixes. Would like to shoot for a 2.1.0 release.

🇺🇸United States jcandan

Am considering this for 2.0.0, or 2.0.1, depending on the speed with which we get a review.

🇺🇸United States jcandan

We have released the bug fixes to 8.x-1.11 and 2.0.0-rc1.

We encourage everyone to at least try 2.0.0-rc1. Feel free to provide feedback here or open new bug tickets.

We will support bug fixes8.x-1.x for at least a couple of weeks after 2.0.0 is released, which we hope to complete in the coming days.

🇺🇸United States jcandan

Also...

so a composer install on a fresh environment no longer works to patch

@alimc29, I wouldn't rely on MR URLs for patches. It is best to use patch files uploaded to the issue or to commit them to your project.

However, don't fret--a release candidate supporting Drupal 11 is forthcoming. :)

🇺🇸United States jcandan

Yeah, sorry, this is in progress. Assigned to me. I am working out some testing kinks in 🐛 Ensure ConfigPageTest actually verifies require_on_publish setting Active first. I plan to get this out today.

🇺🇸United States jcandan

Could we add the following to one of the lists?
🐛 Empty action sets field value to "undefined" Active

🇺🇸United States jcandan

Tested 2.3.1 on Drupal 11.2. This issue does not exist in 2.3.1.

This bug is isolated to 2.2.x releases, and should be addressed in that branch for continued support up to Drupal 10.4 and 11.1 and below core versions.

Drupal 10.4 and 11.1 will receive security coverage until December 2025 ( source ).

🇺🇸United States jcandan

Given 🐛 Link attributes don't save Needs review bug affects both 2.2.x and 2.3.x,

And given the effort here to fix 2.3.x specifically for CKEditor 45+

Can we officially adopt the image/media link attributes bug within the scope of this issue?

This would allow us to laser focus on the issues as they exist differently in each minor version.

The question is, are we willing to change the scope of each, given the amount of work already attempted?

Give it a day or so, if no comments to the contrary, I may go in and update and clarify scopes for each.

🇺🇸United States jcandan

Given this bug affects both 2.2.x and 2.3.x,

And given the effort at 📌 Refactor custom JS for CKEditor5 v45+ Active

Should we make this ticket officially focussed only on 2.2.x?
And should we make the 45+ refactor include this bug in its scope?

This would allow us to laser focus on the issues as they exist differently in each minor version.

The question is, are we willing to change the scope of each, given the amount of work already attempted?

Give it a day or so, if no comments to the contrary, I may go in and update and clarify scopes for each.

🇺🇸United States jcandan

@s_leu, MR !16 is still in draft. Any particular reason? If not, consider removing the Draft prefix from the MR title, and marking Needs Review.

🇺🇸United States jcandan

Agreed. It seems this would be fixed by #3534699.

🇺🇸United States jcandan

I am not sure why, but I am unable to recreate the success I had in #47!

🇺🇸United States jcandan

Confirmed text link attributes all persist as expected with MR !37 on 2.3.x with Drupal 11.2.2.

However, MR !37 breaks image link attributes which, according to #3349389-47: Fix image/media link attributes being lost , all but target="_blank" have been shown working in 2.3.1.

🇺🇸United States jcandan

@ndviet, just a side-note: you may enter issue links as [#123456] and [#123456-7] to take advantage of drupal.org issue and comment linking, and status highlighting.

🇺🇸United States jcandan

2.3.0+ has been released alongside 2.2.0+, and there is now a 2.3.x dev branch to support CKEditor 45 ( #3519380-4: Editor Advanced link 2.3.0 release plan ). This issue is obsolete, and the 2.x branch should be deleted (instead of deleting 2.2.x) to support the multiple minor versions currently supported. Additional reasoning can be found at 🐛 Fix Composer stable, caret constraint installs dev release Active .

🇺🇸United States jcandan

Testing editor_advanced_link 2.3.1 on Drupal 11.2.2 reveals this is almost fixed. Attributes remain except target="_blank".

🇺🇸United States jcandan

Right. It seems Twitter/X is no longer supporting the widgets.js embed as an alternative to the paid basic tier API. I have not seen any official announcement to this specifically, but at this point I think it is safe to assume they've turned off support for this method.

🇺🇸United States jcandan

@estebanvalerio.h, I don't the the README has been updated.

Looking at the commit list for README.md, I see where @adam-vessey got the line from cc493bf7. But the conditional he suggested hasn't been added.

🇺🇸United States jcandan

It’s a beautiful thing.

🇺🇸United States jcandan

Thanks for contributing!

I think confg was supposed to be config.

Could we get that corrected on the readme and removed from the spell list?

🇺🇸United States jcandan

We're getting closer to a proper roadmap. I have placed this in the queue for 2.1.0.

🇺🇸United States jcandan

This last commit introduced new lint errors; perhaps used the wrong eslint settings.

If you revert ecb04525 - fixed eslint issues, you will only need to address the following:

/builds/issue/require_on_publish-3530547/web/modules/custom/require_on_publish-3530547/js/states.js
   8:15  error  Identifier 'legend_span' is not in camel case  camelcase
  23:13  error  Identifier 'legend_span' is not in camel case  camelcase
  34:13  error  Identifier 'legend_span' is not in camel case  camelcase
🇺🇸United States jcandan

Let's limit the lint fixes to the changed or newly introduced files, please.

The following already exist to fix the lint failures:

🇺🇸United States jcandan

We'll drop Drupal 9 support. We'll go to 2.0.0.

🇺🇸United States jcandan

Tests pass. A few implementation updates would no longer support v16.0.0.

Not tested support with Telephone Validation nor Telephone Formatter . Would appreciate it if someone could take a gander at that.

🇺🇸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.

Production build 0.71.5 2024