Account created on 20 September 2008, over 15 years ago
#

Merge Requests

Recent comments

πŸ‡ΊπŸ‡ΈUnited States generalredneck

FYI I would have post a new relic stack trace, except that the improvements were so fast, new relic didn't keep any of the trace data. We went from 30+ seconds down to ~800ms with this fix on media creation with around 150 image styles and focal point installed.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

This is a simple and creative fix.

The only situation where this "might" fail is removing the image_path_flush($this->uri->value); on instantiation. Looking through the commit history, there is no mention of why they did it on entity creation as well, but it may happen if a crop is added to a field where an existing file entity is stored. This may happen in the case where someone is using a module to update a file with Media Entity File Replace or IMCE β†’ , or if the file entity is programmatically changed somehow (migration?). In these cases a particular style may already have a derivative, but I would guess it would be up to that module to clean them up? I'm grasping at straws honestly, but I want us to test those usecases.

As it is, using with focal point and none of these situations, this works amazingly well and is a vast improvment performance wise, ESPECIALLY on S3 and Pantheon Filesystems.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

Adding a related issue on Crop that's been open for a while.

That doesn't mean that this patch isn't still needed. In order for Webp to correctly and always remove derivatives at the same time the other image derivatives are removed, besides when a full image style is nuked, This is the way to go.

but #3200311: Performance issues due to multiple calls of the image_path_flush() β†’ may help others that are facing a similar problem I'm hitting that is compounding this issue.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

I do want to point out that there are likely ways to make this even more performant, but I'm kinda at a loss. The biggest kicker is that currently the crop module calls ImageStyle::flush() every time a crop entity is saved... including created.

See /media/add/image call on this site on pantheon here

I'm assuming this is because you have things like IMCE where you can browse for existing files on the disk and there MAY be existing derivatives for the styles that exist from the before times or something, and this ensures that we have a clean slate.

I wish however there was a way we could check without it being murder to performance on filesystems like Pantheon's, or S3fs, as running a file_stat call is costly. With the 125 image styles we are using, running this hook on Image Create is costing something like 15 seconds. It was worse with the other code at twice that (which you can see in the new relic in #5.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

Flyke,
Thanks for the quick response.

ExtLink can, and does, cover the usecase this module provides. You can see the comment in πŸ’¬ external_links_new_tab project open to new maintainer applications on 30 Sep, 2021 Fixed expressing so. Additionally, a lot of what you are suggesting are configurable features. Lastly, ExtLink covers links in the content without a need for a module like linkit and thus the need for javascript.

Thanks for your consideration of the issue though. I won't press you any further for change.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

Hey there,
I was working with aasarava to get through this one. So here's the deal:

The solution in πŸ› Using Image Widget Crop in responsive images does not refresh webp image Fixed is even inadequate, and here is why. if there are any changes made to specific images using methods that DON'T alter an entity with a file field, then we still have issues. In addition, ANY changes made to the media entity (even so much as just hitting the save button) rebuilds all the webp derivatives for that image.

That results into a call like this on some filesystems:

Another example where it falls short, lets say any specific module calls ImageStyle::flush("1234.jpg"); Well then in that instance, webp is out of date and shows up incorrectly, because while all the 1234.jpg images got removed, 1234.webp still exists.

This potentially happens when crop entities are updated as well independently of the media entity.

So in order to handle that, we need to be using hook_image_style_flush.

The challenge here though is πŸ› hook_image_style_flush doesn't get the $path passed to Drupal\image\Entity\ImageStyle::flush() method Fixed which is committed to Drupal Core 11, and only immediately available by applying the patch β†’ .

That said, I'm still pushing a MR for this.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

generalredneck β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

Raveen,

I believe the source is getting a link from the previous page when we click printable>print. The link on hover is the same as we are getting as output. There seems to be something in the link handling configuration for subscript reference. Which is taking the link from the printable>print.

I'm sorry but this is incorrect. Also what you are pointing out is what I've reported in another closely related issue πŸ› List of printer-friendly urls has links not printed in output. Active . You will find the source of the printable/print link. It's a link hidden with CSS by printable.

Here's my clearest explanation of what I'm expecting.

the first link marked with [1] should match 1. in the list, [2] with 2, etc.

What's happening instead is [5] should probably be marked as [3] so that we have two [3] on the page. OR https://google.com should be listed twice as Link 3 and 5 in the list at the bottom.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

Anyone else looking to do what I'm doing, if you have theme hook suggestions for display modes... try something like node-printable.html.twig and put something like:

  <div{{ content_attributes }}>
    {{ content|without('printable_navigation') }}
  </div>

In it. This removes the links that are currently hidden via css.

I think an appropriate patch may be to add an if statement in printable_entity_view that checks for $view_mode == 'printable', and just exclude printable_navigation entirely. I don't see that we need those links in the printer friendly version, and if we do, it should likely replace the links to existing entity as opposed to creating a hidden link.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

Ok so Option 1 is out above. Looks like the only way a printable link isn't printed on an entity is if that entity's template explicitly removes the links variable from being rendered.

I may need to refactor this issue to be "Printable doesn't respect the "links" field in the Manage Display section on Node entities...

Custom template or preprocessor it is!

πŸ‡ΊπŸ‡ΈUnited States generalredneck

Well tracked down the Printable links for embedded content. There's css in printable.html.twig that hides the links.

    .node_view ul li{
    display:none;
    }

ugh...

So the options as they stand for me now are:
1) Configure embeded entity view mode such that it doesn't show the "links" section (which would suck if there was links in the link section I needed, but works for me here).
2) use a preprocesssor function that strips the Printable Link out of the Links area for those very specific content types and view modes.

This is going to affect πŸ› Duplicate Links with subscript reference doesn't match up with Printer-friendly URLs list at bottom of page. Active because if we put a subscript next to all links, some of the subscripts are going to appear to be missing.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

generalredneck β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

Given that this issue is 5 years older than my oldest child and it can watch PG-13 without supervision, I created a module that handles the scenario and a few like it.

I present Token Url Plus β†’ .

This module (as the time of this writing) provides the tokens:

[current-page:url-with-query]

The URL of the current page with query string included, if it exists. This is the functionality we were looking for here.

[current-page:url-with-query:without-some-parameters:param1,param2,…]

The URL of the current page with query string included but filtered by removing the provided query parameters. e.g. 'without-some-parameters:utm_campaign,utm_medium,utm_source' would be replaced with the current url, with the three utm parameters filtered out if they existed.

This is advanced usage for what you are looking for here

[current-page:url-with-query:with-some-parameters:param1,param2,…]

The URL of the current page with query string included but filtered to only include the provided query parameters. e.g. 'with-some-parameters:page,category_id' would return a url containing only query parameters, '?page=1&category_id=2' if they existed.

You can use this one with metatags on views to support JUST the query parameters that the view provides.

This will allow Token the module to remain unchanged and take the burden off of the decision for backward compatibility as expressed in #11.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

Alright, a few things:

1) #3 is a perfectly valid solution. it's a quick Regex way to stip off the outsides of the tag. It just made me jumpy since this was in the Filter service, but it's simply grabbing the tag names from the configuration. Something changed in the HTML parsing class that made this quit working. I don't know that this method is any less secure honestly.

2) All the tests pass with this fix (including the unit test around the getAllowedHtmlTags(). You can see this on https://git.drupalcode.org/project/html_title/-/jobs/669556.

I'm going to call this one good.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

This error seems to be caught by the automated tests as well see the latest build https://git.drupalcode.org/project/html_title/-/jobs/669321. I am also hesitant about #3. Upgrading the priority of this one and will tackle it next time I get a chance.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

I rerolled the MR and fixed a coding standards issue.

Getting this in.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

generalredneck β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

Validated that all the new code also passed coding standards and merged this.

Thank you both

πŸ‡ΊπŸ‡ΈUnited States generalredneck

generalredneck β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

First I made some changes to the test as it appears they were written before the modules parameter became "protected".

Second, I added some HTML to the tests to ensure that html_title continues to function as expected there as well.

Lastly, The recent test failures are do to changes in Drupal 10.2.*. This test works solo in 10.2.*. Running the tests (including this one solo) against 10.1.8 work fine, so I'm going to call this one good to go and will work to find the challenges in 10.2. I suspect it's part of πŸ› Only first element tag in configuration form is rendered in title on Drupal 10.2 Active .

πŸ‡ΊπŸ‡ΈUnited States generalredneck

The way we are getting around this is not to use the wikimedia/composer-merge-plugin package. This is a known limitation if the merge plugin is being installed for the very first time and/or a package is asking to merge in a composer.json that is downloaded WITH the package. See https://github.com/wikimedia/composer-merge-plugin/issues/165 and https://github.com/wikimedia/composer-merge-plugin/issues/170

If you want to ensure it works consistently, you will need to add the repositories section to your project's composer.json by copying it from composer.libraries.json and then running composer require northernco/ckeditor5-anchor-drupal

Another alternative could be using asset-packagist.org and pulling in npm-asset/northernco--ckeditor5-anchor-drupal, so that you can always get the latest version as it updates.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

@pradpnayak,
the file you are experiencing problems with was introduced by a patch in ✨ Add "Anchor" button Needs work .

πŸ‡ΊπŸ‡ΈUnited States generalredneck

triple5,

So any changes made to composer.json doesn't take affect when you patch a module because those changes aren't part of the drupal.org packagist. You won't see that change until this patch gets committed into a branch that has a release associated with it or you do some work to either define a custom package that looks like the changed composer.json or commit the composer.json changes into a repo (forking the module) and pointing a repository entry to that.

To try to make it more clear, a packagist consumes a composer.json file to build a package entry (you can see the one for this module at https://packages.drupal.org/files/packages/8/p2/drupal/fakeobjects.json). When it gets to your machine after you download the code, you then patch it. So it's not even considered.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

Looks like there is more to this... I suspect that 3.0.x of this module is not compatible with Search API 4.3.x. The schema.xmls vary wildly and I get errors while searching with it installed like

Apache Tomcat/7.0.68 - Error report HTTP Status 400 - can not use FieldCache on unindexed field: boost_documenttype Status reportmessage can not use FieldCache on unindexed field: boost_documentdescription The request sent by the client was syntactically incorrect.Apache Tomcat/7.0.68

Looking through schema.xml, there is infact not any boost_document field type installed.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

Just in case a patch is needed:

πŸ‡ΊπŸ‡ΈUnited States generalredneck

Based on the patch and the issue that aasarava provided... I think this is a duplicate of πŸ› Drupal.t() does not respect locale_custom_strings Fixed

πŸ‡ΊπŸ‡ΈUnited States generalredneck

Rerolling#58 with svg icons to make for a better patching experience. Removes the extraneous patch file also.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

So a few things... the latest patch only works with the patch command. git apply complains of

$ git apply editor_advanced_link-add_anchor_button-2595337-58.patch
editor_advanced_link-add_anchor_button-2595337-58.patch:22: trailing whitespace.
οΏ½PNG
editor_advanced_link-add_anchor_button-2595337-58.patch:32: trailing whitespace.
οΏ½PNG
editor_advanced_link-add_anchor_button-2595337-58.patch:41: trailing whitespace.
οΏ½PNG
editor_advanced_link-add_anchor_button-2595337-58.patch:52: trailing whitespace.
οΏ½PNG
error: cannot apply binary patch to 'editor_advanced_link-add_anchor_button-2595337-54.patch' without full index line
error: editor_advanced_link-add_anchor_button-2595337-54.patch: patch does not apply

Additionally, this patch includes another patch in it

$ patch -p1 < editor_advanced_link-add_anchor_button-2595337-58.patch
patching file editor_advanced_link.routing.yml
patching file js/plugins/anchor/icons/anchor-rtl.png
patching file js/plugins/anchor/icons/anchor.png
patching file js/plugins/anchor/icons/hidpi/anchor-rtl.png
patching file js/plugins/anchor/icons/hidpi/anchor.png
patching file js/plugins/anchor/plugin.js
patching file src/Form/EditorAnchorDialog.php
patching file src/Plugin/CKEditorPlugin/AnchorLink.php
patching file editor_advanced_link-add_anchor_button-2595337-54.patch

Lastly, the images don't load in the browser or in the OS.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

A note on the PNG problems... maybe use as SVG instead?

πŸ‡ΊπŸ‡ΈUnited States generalredneck

blah, this was caused by a patch from ✨ Add "Anchor" button Needs work . Marking this as closed dup. sorry about the bother.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

Rajab,

I hope I didn't offend, I was trying to help understanding, not beat an admission of guilt out of you. I hope that I'm just reading too much into your words, but If there are any hard feelings, lets talk, and feel free to slack with me on ways I can improve the way I communicate.

You are not "wrong" and I can see where you come from. Thanks for working with me on this.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

On the contrary, it is mentioned in the 10.2.0 development cycle:
https://www.drupal.org/about/core/policies/core-release-cycles/schedule#... β†’

Week of December 11, 2023 (UTC)

Drupal 10.2.0 released.
End of security support for 10.0.x.

Again, you say

~10.1.0 and later.

I think you mean ^10.1.0 in that statement. because ~10.1.0 is == 10.1.* and would not support later versions of drupal.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

So looking into it... The change was made between 1.2 and 1.3 here: https://git.drupalcode.org/project/entity_browser_enhanced/-/commit/6258...

I think someone got confused between what the tilde ~ and the carat ^

For one... based on that change up until that point, the dependency criteria was TECHINCALLY different but functioned the same because of the way it was written. composer.json contained"drupal/core": "~8.0 || ~9.0 || ~10.0", and entity_browser_enhanced.info.yml contained core_version_requirement: ^8 || ^9 || ^10

I think this lead someone to believe that ^ and ~ were the same. Let me see if I can explain.

~ is the magic operator here. If you write ~10.0 that means >=10.0 <11.0.0. If you write ~10.0.0 that means >=10.0.0 <10.1.0. Another way of looking at it is that using ~ specifies a minimum version, but to go up.

^ on the other hand sticks to non-breaking changes by allowing all versions to upgrade except major versions... So in that example ^10.0.0 and ^10.0 are equivalent. This is the case for all versions greater than 1.0.0. Check out https://getcomposer.org/doc/articles/versions.md#next-significant-releas...

So I'm going to make a MR with the changes.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

I've incorporated the changes @psingh10 made into the MR.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

@psingh10,

Can you summarize the changes you are making from the Merge Reequest. If you didn't know you can start with the patch from there by clicking this link

Based on what I see I did an interdiff between your two last patches and the thing you changed was commented out the "version" key in info.yml

Based on an interdiff between the MR and the patch, the composer.json file is removed, druadmin_lte_theme.libraries.yml added core/once and the version key was commented out. I'm going to get your changes in the MR since some people are using composer to access the MR branch directly (like myself).

πŸ‡ΊπŸ‡ΈUnited States generalredneck

Hey there. Can you provide me with the way you are configuring the module? You don't have to provide the credentials... but I want to validate you got your credentials configured correctly.

Also validate you are using sftp and not ftps here. If you can log in via ssh... you should be able use sftp.

Verify that sftp -P 28 username@myservernam.com works for you also.

I know you told me

Everything is working with any sftp client

But you didn't give me any specifics on how you did testing.

Your settings.php should have the following toward the end

$schemes = [
  'sftpexample' => [
    'driver' => 'sftp',
    'config' => [
      'host' => 'myservername.com',
      'username' => 'username',
      'password' => 'password', // Only one of 'password' or 'privatekey' is needed.
      'privateKey' => 'path/to/or/contents/of/privatekey',
      'root' => '/path/to/root',

      // Optional
      'port' => 28,
      'timeout' => 10,
    ],
    'cache' => TRUE, // Cache filesystem metadata.
  ],
];

$settings['flysystem'] = $schemes;

Note if you are testing on a machine with the correct private key configured for your Linux user... you may find that even providing the wrong password works cause your key matches...

I'm just trying to shotgun this.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

Hey there @thomasmurphy,

So sorting on multivalue fields as a whole doesn't work like this. Adding any kind of sort breaks the rows down into their individual results reguardless of what you have in the "Fields" section of the view...

For example, you might have this:

But as soon as you add a sort clause on a field with multiple cardinality, you get result rows for each value represented in that field. I've got the query turned on so you can see how adding a sort changes the query.

The multiple field thing only works because the field handler for fields have that special ability, So core itself doesn't even have the ability to sort those multivalue fields

All that said, if you use the views_sort_null, you can make that one item appear at the top and the bottom... Additionally, you can use aggregation to narrow the results down to just the nids you want... So something like this... (note you can use this method for other types of sorts too.

So I don't think this is a views_sort_null_field specific issue, but we can totally throw hints out there.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

@ashique12009,

I feel that the Drupal 9 version has been tested and tried and that with #3220598: Fix compatibility with Drupal 9 in the main 8.x-1.x branch β†’ you will find that issues have probably been fixed. This commit did happen in September after you originally reported this issue, so it's likely.

If you could open an issue with some specifics if you are still having the issue, that would be awesome. I'll do my best to help you there.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

Rejoice 8.x-1.2 has been released.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

@DamienMcKenna,

I got most of that branching cleaned up... 9.1.x is tied to a dev release which is unfortunate.

Everyone else,
Testing the fixes here now and If all is well I'll create a release.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

making sure credit is given

πŸ‡ΊπŸ‡ΈUnited States generalredneck

Alright, gave this a thorough testing, got it committed. going to cut a release. marking as fixed

πŸ‡ΊπŸ‡ΈUnited States generalredneck

Marking this as "needs review"

πŸ‡ΊπŸ‡ΈUnited States generalredneck

I think this would be a good candidate for an hook_alter()

πŸ‡ΊπŸ‡ΈUnited States generalredneck

@Shreya_th,

Would you mind explaining the changes made at https://git.drupalcode.org/project/views_natural_sort/-/merge_requests/1...

diff --git a/src/ViewsNaturalSortService.php b/src/ViewsNaturalSortService.php
index 2603a25cd29f244f935a690e62aeee4a8dd94609..b7106713616b2a231dc149b43c601328f8679efa 100644
--- a/src/ViewsNaturalSortService.php
+++ b/src/ViewsNaturalSortService.php
@@ -313,13 +408,13 @@ class ViewsNaturalSortService {
   public function getViewsBaseTable($fieldDefinition) {
     $entityType = $this->entityTypeManager->getDefinition($fieldDefinition->getTargetEntityTypeId());
     $base_table = $entityType->getBaseTable() ?: $entityType->id();
-    $views_revision_base_table = NULL;
+    // $views_revision_base_table = NULL;
     $revisionable = $entityType->isRevisionable();
-    $base_field = $entityType->getKey('id');
-
-    $revision_table = '';
+    // $base_field = $entityType->getKey('id');
+    // $revision_table = '';
     if ($revisionable) {
-      $revision_table = $entityType->getRevisionTable() ?: $entityType->id() . '_revision';
+      // $revision_table = $entityType->getRevisionTable() ?:
+      // $entityType->id() . '_revision';
     }
 
     $translatable = $entityType->isTranslatable();
@@ -332,12 +427,12 @@ class ViewsNaturalSortService {
     // have a revision table name set in
     // \Drupal\Core\Entity\Sql\SqlContentEntityStorage::initTableLayout() so we
     // apply the same kind of logic.
-    $revision_data_table = '';
+    // $revision_data_table = '';.
     if ($revisionable && $translatable) {
-      $revision_data_table = $entityType->getRevisionDataTable() ?: $entityType->id() . '_field_revision';
+      // $revision_data_table = $entityType->getRevisionDataTable() ?:
+      // $entityType->id() . '_field_revision';
     }
-    $revision_field = $entityType->getKey('revision');
-
+    // $revision_field = $entityType->getKey('revision');
     $views_base_table = $base_table;
     if ($data_table) {
       $views_base_table = $data_table;

It appears that you are adding some commented out lines of code for some reason.

I didn't expect you to go through the whole module and fix all the changes that failed coding standards up until now but thank you.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

Shreya_th,
COnsider using phpcs with the DrupalPractice ruleset. Here is the full list of coding standards infractions introduced by this PR.

FILE: /home/allan/workspace/d10/web/modules/contrib/views_natural_sort/src/ViewsNaturalSortService.php
-----------------------------------------------------------------------------------------------------------------------
FOUND 24 ERRORS AFFECTING 21 LINES
-----------------------------------------------------------------------------------------------------------------------
  31 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 0
  31 | ERROR | [x] Whitespace found at end of line
  33 | ERROR | [x] Whitespace found at end of line
  34 | ERROR | [x] Expected 1 space(s) before asterisk; 2 found
  34 | ERROR | [x] Expected 1 space after asterisk; 2 found
  35 | ERROR | [x] Expected 1 space(s) before asterisk; 0 found
  36 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 0
  38 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 0
  39 | ERROR | [x] Expected 1 space(s) before asterisk; 3 found
  40 | ERROR | [x] Expected 1 space(s) before asterisk; 3 found
  41 | ERROR | [x] Expected 1 space(s) before asterisk; 3 found
  42 | ERROR | [x] Expected 1 space(s) before asterisk; 3 found
  69 | ERROR | [x] Expected "\Drupal\Core\Queue\QueueFactory" but found "\Drupal\Core\Queue\QueueFactory;" for @var
     |       |     tag in member variable comment
  76 | ERROR | [x] Expected "\Drupal\Core\Queue\QueueWorkerManagerInterface" but found
     |       |     "\Drupal\Core\Queue\QueueWorkerManagerInterface;" for @var tag in member variable comment
 108 | ERROR | [ ] Parameter $loggerFactory is not described in comment
 110 | ERROR | [x] Whitespace found at end of line
 115 | ERROR | [ ] Missing parameter name
 117 | ERROR | [ ] Doc comment for parameter $transformationManager does not match actual variable name $translator
 132 | ERROR | [x] Parameter comment indentation must be 3 spaces, found 1 spaces
 134 | ERROR | [x] Parameter comment indentation must be 3 spaces, found 1 spaces
 135 | ERROR | [x] Whitespace found at end of line
 136 | ERROR | [x] Additional blank lines found at end of doc comment
 137 | ERROR | [ ] Expected type hint "ConfigFactoryInterface"; found "ConfigFactory" for $configFactory
 137 | ERROR | [ ] Expected type hint "ViewsDataInterface"; found "ViewsData" for $viewsData
-----------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 19 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-----------------------------------------------------------------------------------------------------------------------
πŸ‡ΊπŸ‡ΈUnited States generalredneck

@Shreya_th,
Thanks for making the changes. You got some code formatting issues, but otherwise, it's good to go. If you will make the changes, I would love to merge this.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

Awesome @munchmunch,

Yall will have a new release come this Friday. I'll take Damien's comment under advisement too.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

The commit appears to work for D9.5.10. It redirects as it's supposed to and even respects when a different ?destination=/some/path is passed to the form. I'd call this good minus specific tests provided the maintainer wants those to be a part of this work. Given that, the fix for this has already been commited to 8.x-2.x, so I'm inclined to vote we don't make this linger and create a new task for creating the test.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

Everyone,
So I was upgrading to D10, and we had been using #107.

Obviously when I tried to apply it to the 8.x-2.x branch locally via composer, the patch failed. So I came here to reroll it.

That said, I got to looking at the latest commit, and I've found that the Reason VladimirAus's MR shows up as empty is because #25 has been merged and updated for D10. If you look in πŸ“Œ Automated Drupal 10 compatibility fixes Fixed , you will see where VladimirAus is asking everyone to review that commit and validate it function's correctly so we can have a new version.

So in theory, provided my tests work here in a little bit, this could be marked as "Fixed" provided we want to release without tests of the redirect functionality.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

This will likely not be touched... The release notes for 2.0.0-beta1 β†’ says

This is the first release with D10, claro, and gin compatibility.

Support for seven and thunder_admin was removed. Also, the split functionality was removed, because it hardly depends on ckEditor 4, and would need a complete rewrite.

So it needs to probably be marked as "fixed" or won't fix

πŸ‡ΊπŸ‡ΈUnited States generalredneck

Taking this and throwing it into an update hook on a custom module works great too. just remember to use `drush cex` after, just like any normal workflow.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

I see now that you are actually already a maintainer now. I'm going to mark this as fixed to not further confuse others.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

@DieterHolvoet

Per Item #6 on How to become project owner, maintainer, or co-maintainer > Becoming owner, maintainer, or co-maintainer of a project β†’ :

If the owner does not reply after two weeks, move the issue to the Drupal.org project ownership issue queue, by changing the issue's Project field to Drupal.org project ownership. Please include a link to the project page.
Site moderators will normally try to contact the owner as an extra safeguard unless it is evident the owner is no longer active in the Drupal community, giving to the contacted users two weeks for replying to the message they sent via the Contact tab.

It has been over 2 weeks since you originally contacted the maintainers. This means you should probably escalate this to Drupal.org project ownership β†’ by following the instructions above.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

@Koustubh Dudhe

Awesome, I just noticed this today... Thanks for the work. I did a review of your MR.

on https://git.drupalcode.org/issue/emulsify_drupal-3352511/-/compare/2.x.....

This is unlikely true as the theme relies on componenets ^2.1 which has a requirement of ^8.7.7 || ^9. I would go so far as to say we should probably drop D8 support as of this release.

That said, you changed the package name in composer.json. I don't know if that is going to be a possibility or not given that this repo is mirrored on Github. I'm not sure what the repercussions of that would be.

Throwing this at the other emulsify devs.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

I made a couple of commits to the PR so we can use it as a "repository" while the maintainers are absent. composer.json was misnamed as compose.json and it didn't pass validation because it was missing the closing curly brace.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

generalredneck β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

Awesome,
If there's any particular ways yall would like to collaborate or have any ideas on processes for how releases should be made, I want to be mindful and respect that. In other words I want my responsibilities to be clear so I don't step on toes. If it's ok that I take on full maintainership and yall want to pass the torch, that's cool too. Just as long as it's clear. Feel free to reach out on slack if yall want to talk this over in a synchronous way.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

I've reached out to both Joachim and miiimooo via their contact forms to make sure that just in case they don't have this issue queue followed to email them of new issues, that they are aware of my request above. I had copies of the emails sent to me as well.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

No problem. I can't promise it will always be that way as I am a maintainer on several modules.

I kinda expected to have something like this on this one since it was a large release, I'm unfamiliar with the specifics, and we have 0 automated tests. I'm really surprised that one didn't' show up on rector and that explains a fix I saw in the D10 MRs that someone else made... though this fix is MUCH preferred.

Thank you for being on top of testing it out and getting us a contributed fix quickly as well.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

Pushing this out as 2.0.1 since it's pretty much a hotfix. Should be ready soon.

πŸ‡ΊπŸ‡ΈUnited States generalredneck

@ipwa,
A reminder also when you have posted work, Please mark it as "Needs Review". This will help me prioritize the queue for what needs testing. I'll try to get to it as my email will allow me since I get notified for all new issues in this queue, but you know... at a glance this looks like an open ticket with no solutions to review. Figure it was just a slip though.

All that said, I tested the solution and looks like we are good to go!

πŸ‡ΊπŸ‡ΈUnited States generalredneck

Ammending the steps to reproduce as the original is inaccurate.

Production build https://api.contrib.social 0.61.6-2-g546bc20