Balneário Camboriú
Account created on 9 July 2018, over 6 years ago
#

Merge Requests

Recent comments

🇧🇷Brazil dungahk Balneário Camboriú

I have been looking at this and there is a bigger problem with that part of the code. The alt text is not being updated even with that patch or any other code change that removes the call to the shopify_fetch_alt function.

I'm investigating the best way to fix the issue.

🇧🇷Brazil dungahk Balneário Camboriú

I merged an MR I created based on the MR created by alt.dev and released this as part of 2.0.0-alpha1 .

Feedback will be appreciated in the form of new issues.

🇧🇷Brazil dungahk Balneário Camboriú

I have repurposed the 2.0.x branch to be the D10 compatible branch, while being the same as the 8.x-1.x branch (still not using the Shopify official SDK).

🇧🇷Brazil dungahk Balneário Camboriú

I've merged the MR.

🇧🇷Brazil dungahk Balneário Camboriú

For what is worth it, I've tried the index mentioned in #34 and it improved a lot the performance of the query.

Before the index:

51 rows in set (57.57 sec)

After the index:

51 rows in set (17.26 sec)

The index query for reference: ALTER TABLE file_managed ADD INDEX groupidx(fid, filename, filemime, filesize, status, created, changed);

And this is the select query: SELECT file_managed.fid AS fid, file_managed.filename AS file_managed_filename, file_managed.filemime AS file_managed_filemime, file_managed.filesize AS file_managed_filesize, file_managed.status AS file_managed_status, file_managed.created AS file_managed_created, file_managed.changed AS file_managed_changed, SUM(file_usage_file_managed.count) AS file_usage_file_managed_count, MIN(file_managed.fid) AS fid_1 FROM file_managed file_managed LEFT JOIN file_usage file_usage_file_managed ON file_managed.fid = file_usage_file_managed.fid GROUP BY file_managed.fid, file_managed_filename, file_managed_filemime, file_managed_filesize, file_managed_status, file_managed_created, file_managed_changed ORDER BY file_managed_changed DESC LIMIT 51 OFFSET 0;

🇧🇷Brazil dungahk Balneário Camboriú

Update title to match the error exactly

🇧🇷Brazil dungahk Balneário Camboriú
🇧🇷Brazil dungahk Balneário Camboriú
🇧🇷Brazil dungahk Balneário Camboriú

Hi,

This report is for the TFA module, not the diff module.

🇧🇷Brazil dungahk Balneário Camboriú

Attaching a new version of the patch

🇧🇷Brazil dungahk Balneário Camboriú

I had the same issue and I ended up just setting a value to the plaintext parameter.

I agree the module should handle that better though, either validating the rule to make sure the value is not empty or as #7 suggested, add a check on the value.

🇧🇷Brazil dungahk Balneário Camboriú

I don't understand how this issue got to this state, this might sound rude but it looks like no one who left a comment read the issue summary as it clearly states that, yes, there is/was another issue proposing a similar solution and points out the limitation with that solution and adds new functionality on top of that.

What is in the dev branch is not the same thing that this issue is proposing.

The patch I uploaded on #9 is up to date with 4.0.x.

I hope everything is clear now so hoping for feedback on the changes the patch is proposing.

🇧🇷Brazil dungahk Balneário Camboriú

Stumbled across this so just leaving an up to date link to the commit: https://git.drupalcode.org/project/site_audit/-/commit/2030154

🇧🇷Brazil dungahk Balneário Camboriú

I'm coming from https://www.drupal.org/drupalorg/blog/introducing-the-bounty-program and just read everything so I'd like to make a summary of the current state of this issue. First of all, I'm somewhat "ignoring" the comments and patches before #67 🐛 Composer scaffolding fails when permissions on default.settings.yml or default.settings.php is not writable. Needs work as they happened before 3103090 was implemented.

First of all, these are the scenarios that could cause this issue to happen:

  1. The site builder modifies the files ( #67 🐛 Composer scaffolding fails when permissions on default.settings.yml or default.settings.php is not writable. Needs work )
  2. The site builder removes the files ( #67 🐛 Composer scaffolding fails when permissions on default.settings.yml or default.settings.php is not writable. Needs work )
  3. Core updates the files ( #92 🐛 Composer scaffolding fails when permissions on default.settings.yml or default.settings.php is not writable. Needs work )

These are the workarounds:

1. composer.json changes

Documentation: https://www.drupal.org/docs/develop/using-composer/using-drupals-compose...
Code:

"extra": {
  drupal-scaffold": {
    "file-mapping": {
      "[web-root]/sites/default/default.services.yml": false,
      "[web-root]/sites/default/default.settings.php": false
    }
  }
}

See #73 🐛 Composer scaffolding fails when permissions on default.settings.yml or default.settings.php is not writable. Needs work .

The following are the suggestions to fix the issue:

1. Trying harder

Suggested by greg.1.anderson on #67 🐛 Composer scaffolding fails when permissions on default.settings.yml or default.settings.php is not writable. Needs work :

a simpler strategy of simply trying harder would be appropriate. i.e. the scaffold plugin could always try to make a write-protected file writable first, and only fail if it does not have permission to do that.

My thoughts

It's a good idea, but we risk leaving the folder/file writable if the composer command fails in the middle of the process, I know it's unlikely, but imagine a scenario where we change the folder/file permission to writable and composer fails because of OOM before reverting that change, if you try to run the command again it will just succeed but it won't "unharden" (change the permissions back to what they were before) as it won't know what permissions they were before.

2. Optional scaffold files

Suggested by greg.1.anderson on #67 🐛 Composer scaffolding fails when permissions on default.settings.yml or default.settings.php is not writable. Needs work :

If a scaffold file is optional, then any attempt to modify the file that fails becomes a warning instead of an error. This idea could also be done in a follow-on issue.

#95 🐛 Composer scaffolding fails when permissions on default.settings.yml or default.settings.php is not writable. Needs work also metions the same solution.

My thoughts

I like the idea and I think it also links to the next suggestion.

3. Don't even try

Suggested by tedbow on #97 🐛 Composer scaffolding fails when permissions on default.settings.yml or default.settings.php is not writable. Needs work :
I can't find a quote so here's a summary: He suggests rethinking the need for scaffolding sites/default/default.* files to avoid folder permission issues and facilitate the Automatic Updates initiative.

TLDR: Don't scaffold these files at all.

My thoughts

I like this idea. There is a concern raised about this though which is cases where the Drupal installation is actually using that file.

4. Symlink

Suggested by Wim Leers on #102 🐛 Composer scaffolding fails when permissions on default.settings.yml or default.settings.php is not writable. Needs work :

why don't we symlink them from core/assets/scaffold/files/default.*? That way, they're always up-to-date, and we never have to deal with permissions ever again.

My thoughts

I like this idea but the same concern from the previous suggestion applies here I believe, if an installation is using the file how is it gonna update the contents of it if it's not actually a file, only a link? Developers have the option to append files as part of the scaffolding process as well.

5. Templates folder

Suggested by xmacinfo on #105 🐛 Composer scaffolding fails when permissions on default.settings.yml or default.settings.php is not writable. Needs work :

Have Composer scaffold core/assets/defaults/default.settings.php to sites/templates/default.settings.php.

My thoughts

We need to remember that the sites directory is used by multisites Drupal installations and that would mean if there is an installation called "examples" or "templates" they would now have new files into their folder "out of nowhere".

6. md or txt

Suggested by xmacinfo on #112 🐛 Composer scaffolding fails when permissions on default.settings.yml or default.settings.php is not writable. Needs work :

remove the whole default.settings.php file altogether and document the default values in the README.md file

if we want to keep that file, we can rename it to default.settings.txt so that PHP will not try to execute it.

we need Composer Scaffold to store that file outside of the default protected folder.

My thoughts

Someone else raised the concern about using those two extensions that they might become available to end-users, as webservers are usually configured to block .php files but to allow .md and .txt files, so this comes with security risks attached.

My Suggestion

Do what suggestion number 3 is saying, but because it's possibly not BC for some use case scenarios, do what suggestion number 2 is saying for now until we release a new major version (Drupal 11?), have a list of files that are "optional" (naming to be discussed), try to modify them, but don't fail the whole process, only display a deprecation warning linking to a piece of documentation of why this warning is being displayed and what can be done to get rid of it.
This means we would have to update all the references to these files, such as the core/Install.txt file and others.

Another potential solution would be to simply do suggestion 2 as part of this issue, and the rest as part of a follow-up. Just to make sure we resolve the actual problem of devs not being able to run composer commands as soon as possible.

🇧🇷Brazil dungahk Balneário Camboriú

Hi Remco,

Thank you for raising this. I believe https://www.drupal.org/u/matroskeen has started this work as part of https://www.drupal.org/project/shopify/issues/3374457 📌 Create a new class Shopify with getClient() method Fixed

I'm not sure how far is he in the implementation, but he created the 2.0.x branch with his changes.

Can you try contacting him directly? Either via https://www.drupal.org/user/3426249/contact or Drupal Slack.

🇧🇷Brazil dungahk Balneário Camboriú

@Eric_A That issue is different, I initially thought they were the same too, but this one is specifically about making sure inactive Twig compiled files are deleted across multiple webheads, that other issue did not fix this, one still needs to delete them separately.

🇧🇷Brazil dungahk Balneário Camboriú

Hi netgeek123,

Can you let us know how to replicate the issue?

🇧🇷Brazil dungahk Balneário Camboriú

The issue must be on 1.x because 2.x is not using donutdan4114/shopify anymore.

🇧🇷Brazil dungahk Balneário Camboriú

Just updating the status.

🇧🇷Brazil dungahk Balneário Camboriú

Attaching a patch with only the D10 changes since we don't have any release with them.

🇧🇷Brazil dungahk Balneário Camboriú

Attaching a patch for version 4.0.0

🇧🇷Brazil dungahk Balneário Camboriú

The patches provided didn't work for me, uploading the one that did and the diff from #7

🇧🇷Brazil dungahk Balneário Camboriú

Rolling patch from #6 to v2

Note: this does not address the comments from #7

🇧🇷Brazil dungahk Balneário Camboriú

dungahk made their first commit to this issue’s fork.

Production build 0.71.5 2024