driskell Github contribution chart
driskell Github Stats
driskell Most Used Languages

Activity

03 Oct 2022

Driskell

parserPreset is overwritten when extending multiple configurations

I am attempting to replace the default headerPattern and headerCorrespondence values of @commitlint/config-conventional by using a plugin module.

My plugin module looks like this:

index.js

module.exports = {
  parserPreset: {
    parserOpts: {
      headerPattern: /^(?:([A-Z]{2,}-\d+(?:, [A-Z]{2,}-\d+)*) \| ){1}(\w*)(?:\((\w*)\))?!?: (.+)$/,
      headerCorrespondence: [`jiraIds`, `type`, `scope`, `subject`],
    },
  },
  rules: {
    'header-max-length': [2, `always`, 110],
    'jira-empty': [2, `always`],
  },
  plugins: [
    {
      rules: {
        'jira-empty': ({ jiraIds }) => {
          const test = jiraIds ? true : false;
          return [
            test,
            `jira id(s) may not be empty`,
          ]
        },
      }
    }
  ]
} 

package.json

{
  "name": "@acse/commitlint-config",
  "version": "0.4.0",
  "description": "ACSE Commitlint Config",
  "files": [
    "index.js"
  ],
  "keywords": [
    "acse",
    "commitlint"
  ],
  "peerDependencies": {
    "@commitlint/cli": "^17.0.3",
    "@commitlint/config-conventional": "^17.0.3"
  }
} 

Expected Behavior

From a test git repository with @commitlint/cli, @commitlint/config-conventional, and @acse/commitlint-config (the plugin module) installed locally, run:

echo 'JIRA-111, JIRA-222 | feat(scope): subject' | ./node_modules/.bin/commitlint -x @commitlint/config-conventional @acse/commitlint-config 

My expectation would be that this command exits in success (since that header line passes the headerPattern regex set in the plugin module's config.

Current Behavior

Instead that command exits in failure because the parserPreset in the resolved config has been overwritten by the parserPreset that @commitlint/config-conventional sets (note the addition of --print-config):

# echo 'JIRA-111, JIRA-2222 | feat(scope): subject' | ./node_modules/.bin/commitlint -x @commitlint/config-conventional @acse/commitlint-config --print-config

{
  extends: [ '@commitlint/config-conventional', '@acse/commitlint-config' ],
  formatter: '/home/voxmachina/dev/test/test/node_modules/@commitlint/format/lib/index.js',
  parserPreset: {
    parserOpts: {
      headerPattern: /^(\w*)(?:\((.*)\))?!?: (.*)$/,
      breakingHeaderPattern: /^(\w*)(?:\((.*)\))?!: (.*)$/,
      headerCorrespondence: [ 'type', 'scope', 'subject' ],
      noteKeywords: [ 'BREAKING CHANGE', 'BREAKING-CHANGE' ],
      revertPattern: /^(?:Revert|revert:)\s"?([\s\S]+?)"?\s*This reverts commit (\w*)\./i,
      revertCorrespondence: [ 'header', 'hash' ],
      issuePrefixes: [ '#' ]
    },
    name: 'conventional-changelog-conventionalcommits',
    path: './node_modules/conventional-changelog-conventionalcommits/index.js'
  },
  ignores: undefined,
  defaultIgnores: undefined,
  plugins: { local: { rules: { 'jira-empty': [Function: jira-empty] } } },
  rules: {
    'body-leading-blank': [ 1, 'always' ],
    'body-max-line-length': [ 2, 'always', 100 ],
    'footer-leading-blank': [ 1, 'always' ],
    'footer-max-line-length': [ 2, 'always', 100 ],
    'header-max-length': [ 2, 'always', 110 ],
    'subject-case': [
      2,
      'never',
      [ 'sentence-case', 'start-case', 'pascal-case', 'upper-case' ]
    ],
    'subject-empty': [ 2, 'never' ],
    'subject-full-stop': [ 2, 'never', '.' ],
    'type-case': [ 2, 'always', 'lower-case' ],
    'type-empty': [ 2, 'never' ],
    'type-enum': [
      2,
      'always',
      [
        'build',  'chore',
        'ci',     'docs',
        'feat',   'fix',
        'perf',   'refactor',
        'revert', 'style',
        'test'
      ]
    ],
    'jira-empty': [ 2, 'always' ]
  },
  helpUrl: 'https://github.com/conventional-changelog/commitlint/#what-is-commitlint',
  prompt: {
    questions: {
      type: {
        description: "Select the type of change that you're committing",
        enum: {
          feat: {
            description: 'A new feature',
            title: 'Features',
            emoji: '✨'
          },
          fix: { description: 'A bug fix', title: 'Bug Fixes', emoji: 'πŸ›' },
          docs: {
            description: 'Documentation only changes',
            title: 'Documentation',
            emoji: 'πŸ“š'
          },
          style: {
            description: 'Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)',
            title: 'Styles',
            emoji: 'πŸ’Ž'
          },
          refactor: {
            description: 'A code change that neither fixes a bug nor adds a feature',
            title: 'Code Refactoring',
            emoji: 'πŸ“¦'
          },
          perf: {
            description: 'A code change that improves performance',
            title: 'Performance Improvements',
            emoji: 'πŸš€'
          },
          test: {
            description: 'Adding missing tests or correcting existing tests',
            title: 'Tests',
            emoji: '🚨'
          },
          build: {
            description: 'Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)',
            title: 'Builds',
            emoji: 'πŸ› '
          },
          ci: {
            description: 'Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)',
            title: 'Continuous Integrations',
            emoji: 'βš™οΈ'
          },
          chore: {
            description: "Other changes that don't modify src or test files",
            title: 'Chores',
            emoji: '♻️'
          },
          revert: {
            description: 'Reverts a previous commit',
            title: 'Reverts',
            emoji: 'πŸ—‘'
          }
        }
      },
      scope: {
        description: 'What is the scope of this change (e.g. component or file name)'
      },
      subject: {
        description: 'Write a short, imperative tense description of the change'
      },
      body: { description: 'Provide a longer description of the change' },
      isBreaking: { description: 'Are there any breaking changes?' },
      breakingBody: {
        description: 'A BREAKING CHANGE commit requires a body. Please enter a longer description of the commit itself'
      },
      breaking: { description: 'Describe the breaking changes' },
      isIssueAffected: { description: 'Does this change affect any open issues?' },
      issuesBody: {
        description: 'If issues are closed, the commit requires a body. Please enter a longer description of the commit itself'
      },
      issues: {
        description: 'Add issue references (e.g. "fix #123", "re #123".)'
      }
    }
  }
} 

You can see that it does correctly merge the local plugin definition and additional rule from the plugin module, but it overwrites the parserPreset value entirely.

If I don't additionally extend the configuration with with @commitlint/config-conventional I do get the correct headerPattern and headerCorrespondence values, but then I lose everything else extending @commitlint/config-conventional gives me (all the default rules, prompt definitions, etc):

# echo 'JIRA-111, JIRA-222 | feat(scope): subject' | ./node_modules/.bin/commitlint -x @acse/commitlint-config --print-config

{
  extends: [ '@acse/commitlint-config' ],
  formatter: '/home/voxmachina/dev/test/test/node_modules/@commitlint/format/lib/index.js',
  parserPreset: {
    parserOpts: {
      headerPattern: /^(?:([A-Z]{2,}-\d+(?:, [A-Z]{2,}-\d+)*) \| ){1}(\w*)(?:\((\w*)\))?!?: (.+)$/,
      headerCorrespondence: [ 'jiraIds', 'type', 'scope', 'subject' ]
    }
  },
  ignores: undefined,
  defaultIgnores: undefined,
  plugins: { local: { rules: { 'jira-empty': [Function: jira-empty] } } },
  rules: {
    'header-max-length': [ 2, 'always', 110 ],
    'jira-empty': [ 2, 'always' ]
  },
  helpUrl: 'https://github.com/conventional-changelog/commitlint/#what-is-commitlint',
  prompt: {}
} 

Affected packages

  • [x] cli
  • [x] config-conventional

Possible Solution

Not sure, but my expectation based on reading the documentation and gut instinct would be that the parserPreset options are merged into the ones provided by packages extended "before" my plugin package (when defining the extensions via the -x option on the commitlint cli, i.e.:

Steps to Reproduce (for bugs)

  1. Create plugin module as detailed above
  2. Create test git repository that installs @commitlint/cli, @commitlint/config-conventional, and <plugin_module> locally
  3. Run the above commands, validating that the parserPreset gets overwritten

Context

Essentially I am attempting to "extend" the default rules and configuration of the @commitlint/config-conventional module with headerPattern and headerCorrespondence values that should inform the parser to parse out a superset of values that are expected from the header (jiraIds, type, scope, subject).

Your Environment

| Executable | Version | | ---------------------: | :------ | | commitlint --version | 17.0.3 | | git --version | 2.25.1 | | node --version | 16.14.2 |

Forked On 03 Oct 2022 at 01:37:14

Driskell

I am finding this impossible. With the comment above the parseOpts ends up ignored as it's not within a parserPreset.

It seems the @commitlint/resolve-extends is unable to merge parserOpts because when you are referencing @commitlint/config-conventional it's now a function that is returned for parserOpts, which of course can't merge so it just overwrites.

Commented On 03 Oct 2022 at 01:37:14
Issue Comment

Driskell

SHA256 in README is wrong for 2022-03-15

SHA256 in README is wrong for 2022-03-15

Setup

The README says that the SHASUM is 8f65952c3435f441e7f793941d5162d3ec2033a9ef82722ff1da67a2ef860a2f.

It's actually ed66dc9e71aed7602b9ae548f8535131831026f934f19f868fdefbe6a3ab9bf9. Unclear where the discrepancy is from.

Steps to Reproduce Issue

$ wget https://raw.githubusercontent.com/saltstack/salt-bootstrap/v2022.03.15/bootstrap-salt.sh
$ shasum -a256 bootstrap-salt.sh
ed66dc9e71aed7602b9ae548f8535131831026f934f19f868fdefbe6a3ab9bf9  bootstrap-salt.sh 

Versions and Systems

N/A

Forked On 30 Sep 2022 at 11:27:26

Driskell

If you're accessing it using that URL it goes to the tagged version which it seems is not the full release. I do think it used to be recommended to use that URL but yes going forwards I don't think it's the right way and isn't documented anymore.

E.g. for 2022.08.13 you can see the repository file at that tag is still unstable flagged: https://github.com/saltstack/salt-bootstrap/blob/v2022.08.13/bootstrap-salt.sh#L587

You should use the release asset link which has that line removed, and is listed in the GitHub releases downloads: https://github.com/saltstack/salt-bootstrap/releases/download/v2022.03.15/bootstrap-salt.sh

Latest is: https://github.com/saltstack/salt-bootstrap/releases/download/v2022.08.13/bootstrap-salt.sh

Commented On 30 Sep 2022 at 11:27:26
Issue Comment

Driskell

stock alert notification for product which has been already disabled

Preconditions Here is environment I am using

Magento version - 2.3.3 PHP Version 7.2.31 MySQL Version 5.7.18

Steps to reproduce

  1. Make any product out of stock and view the product in front side.
  2. Subscribe for 'Product back in stock' alert.
  3. Go to admin panel and make that product back in stock.
  4. Also disable that product

Expected result Customer should not get mail with alert of product which is disabled

Actual result Customer get mail with alert that product which is already disabled

Forked On 27 Sep 2022 at 09:24:13

Driskell

Reproducible in 2.4.3

  • Make any product out of stock and view the product in front side.
  • Subscribe for 'Product back in stock' alert.
  • Go to admin panel and make that product back in stock.
  • Also disable that product
  • Wait for the product alert schedule to run
  • You get an email

@magento give me 2.4-develop instance

Commented On 27 Sep 2022 at 09:24:13

Driskell

Consistent style in readme

Pushed On 17 Sep 2022 at 01:53:17

Driskell

Clarify git restrictions

Pushed On 17 Sep 2022 at 01:53:17

Driskell

build(deps-dev): bump jest in /npm_and_yarn/helpers

Bumps jest from 27.5.1 to 28.1.0.


updated-dependencies:

  • dependency-name: jest dependency-type: direct:development update-type: version-update:semver-major ...

Signed-off-by: dependabot[bot] support@github.com

Pushed On 17 Sep 2022 at 01:53:17

Driskell

Fix inverted condition

Pushed On 17 Sep 2022 at 01:53:17

Driskell

Trigger commit

Pushed On 17 Sep 2022 at 01:53:17

Driskell

fix DependencyFileNotParseable due to 1MB file limit change

https://github.blog/changelog/2022-05-03-increased-file-size-limit-when-retrieving-file-contents-via-rest-api/

Pushed On 17 Sep 2022 at 01:53:17

Driskell

fix implicit return of the file contents

Pushed On 17 Sep 2022 at 01:53:17

Driskell

Merge pull request #5086 from dependabot/pavera/pin-debase-versions

pin debase version to avoid build errors

Pushed On 17 Sep 2022 at 01:53:17

Driskell

Responding to PR feedback from @mattt

Pushed On 17 Sep 2022 at 01:53:17

Driskell

Merge pull request #5107 from dependabot/jakecoffman/fix-1mb-limit

fix DependencyFileNotParseable due to 1MB file limit change

Pushed On 17 Sep 2022 at 01:53:17

Driskell

v0.185.0

Pushed On 17 Sep 2022 at 01:53:17

Driskell

Merge pull request #5109 from dependabot/v0.185.0-release-notes

v0.185.0

Pushed On 17 Sep 2022 at 01:53:17

Driskell

Fix incomplete URL substring sanitization in bundler shared_bundler_helpers

Pushed On 17 Sep 2022 at 01:53:17

Driskell

Fix incomplete URL substring sanitization in composer version_resolver

Pushed On 17 Sep 2022 at 01:53:17

Driskell

Fix incomplete URL substring sanitization in npm_and_yarn yarn_lockfile_updater

Pushed On 17 Sep 2022 at 01:53:17

Driskell

Fix incomplete URL substring sanitization in python metadata_finder

Pushed On 17 Sep 2022 at 01:53:17

Driskell

Fix incomplete URL substring sanitization in bundler helpers file_parser

Pushed On 17 Sep 2022 at 01:53:17

Driskell

Fix incomplete URL substring sanitization in terraform file_parser

Pushed On 17 Sep 2022 at 01:53:17

Driskell

Add missing require statements for uri module

Pushed On 17 Sep 2022 at 01:53:17

Driskell

Add fixtures to paths-ignore for CodeQL workflow

Pushed On 17 Sep 2022 at 01:53:17

Driskell

Consistent style in readme

Pushed On 17 Sep 2022 at 01:52:40

Driskell

Clarify git restrictions

Pushed On 17 Sep 2022 at 01:52:40

Driskell

build(deps-dev): bump jest in /npm_and_yarn/helpers

Bumps jest from 27.5.1 to 28.1.0.


updated-dependencies:

  • dependency-name: jest dependency-type: direct:development update-type: version-update:semver-major ...

Signed-off-by: dependabot[bot] support@github.com

Pushed On 17 Sep 2022 at 01:52:40

Driskell

Fix inverted condition

Pushed On 17 Sep 2022 at 01:52:40

Driskell

Trigger commit

Pushed On 17 Sep 2022 at 01:52:40

Driskell

fix DependencyFileNotParseable due to 1MB file limit change

https://github.blog/changelog/2022-05-03-increased-file-size-limit-when-retrieving-file-contents-via-rest-api/

Pushed On 17 Sep 2022 at 01:52:40

Driskell

fix implicit return of the file contents

Pushed On 17 Sep 2022 at 01:52:40

Driskell

Merge pull request #5086 from dependabot/pavera/pin-debase-versions

pin debase version to avoid build errors

Pushed On 17 Sep 2022 at 01:52:40

Driskell

Responding to PR feedback from @mattt

Pushed On 17 Sep 2022 at 01:52:40

Driskell

Merge pull request #5107 from dependabot/jakecoffman/fix-1mb-limit

fix DependencyFileNotParseable due to 1MB file limit change

Pushed On 17 Sep 2022 at 01:52:40

Driskell

v0.185.0

Pushed On 17 Sep 2022 at 01:52:40

Driskell

Merge pull request #5109 from dependabot/v0.185.0-release-notes

v0.185.0

Pushed On 17 Sep 2022 at 01:52:40

Driskell

Fix incomplete URL substring sanitization in bundler shared_bundler_helpers

Pushed On 17 Sep 2022 at 01:52:40

Driskell

Fix incomplete URL substring sanitization in composer version_resolver

Pushed On 17 Sep 2022 at 01:52:40

Driskell

Fix incomplete URL substring sanitization in npm_and_yarn yarn_lockfile_updater

Pushed On 17 Sep 2022 at 01:52:40

Driskell

Fix incomplete URL substring sanitization in python metadata_finder

Pushed On 17 Sep 2022 at 01:52:40

Driskell

Fix incomplete URL substring sanitization in bundler helpers file_parser

Pushed On 17 Sep 2022 at 01:52:40

Driskell

Fix incomplete URL substring sanitization in terraform file_parser

Pushed On 17 Sep 2022 at 01:52:40

Driskell

Add missing require statements for uri module

Pushed On 17 Sep 2022 at 01:52:40

Driskell

Add fixtures to paths-ignore for CodeQL workflow

Pushed On 17 Sep 2022 at 01:52:40
Issue Comment

Driskell

See if we can leverage `composer`'s `outdated` command

composer supports an outdated command which we may potentially be able to leverage to see which libraries are outdated.

The output has sections for both direct and transitive deps, and it supports a --major-only flag to show only packages with major version updates.

I am not familiar enough with the Dependabot composer implementation, but I know we use composer as a library for native helpers... if we do check outdated at all, it'd be convenient if we could flip to directly calling composer cli with some of these flags.

See also:

  • https://github.com/dependabot/dependabot-core/issues/5719
  • https://github.com/dependabot/dependabot-core/issues/5720

Forked On 17 Sep 2022 at 12:39:58

Driskell

Outdated would work to check what needs running through the update. But I remember from testing the β€œis there an update available?” part of the dependabot update was not slow at all. If you had a drupal repository with lots of packages with no updates needed it would run through super quick. It only time out if there was updates as each package update would take some time to resolve the package set after updating.

Commented On 17 Sep 2022 at 12:39:58
Issue Comment

Driskell

See if we can leverage `composer` `2.4`'s new `bump` feature

Composer 2.4 added a new bump option: https://blog.packagist.com/composer-2-4/

We may be able to leverage this a bit more directly for bumping versions in composer.json. See the caveats about doing it for libraries on that blog post.

See also:

  • https://github.com/dependabot/dependabot-core/issues/5721

Forked On 17 Sep 2022 at 12:38:21

Driskell

Just to put here what was mentioned elsewhere. I think the bump just increases the json to the currently installed version so doesn’t update anything except the minimum requirement and only updates it what’s already installed

Commented On 17 Sep 2022 at 12:38:21
Issue Comment

Driskell

fix: Fix unpredictable false positive/negative of missing entity access check in function scope due to cache key conflict

Fixes #475

The EntityQueryType was getting cached in function scope for a condition call and because it wasn't handled by dynamic return it reused. So if you had condition before accessCheck and then later on after it, the latter one could fail because it would return the cached without access check and there would be no subsequent access check (it happens before).

Added test case.

Still examining why this doesn't occur in class scope but it seems phpstan has additional cache key when it is just not sure of its granularity.

Forked On 16 Sep 2022 at 05:45:07

Driskell

It did happen in class scope - PR has tests for both that reproduce and the fix solve

Commented On 16 Sep 2022 at 05:45:07
Issue Comment

Driskell

Timeout: error="waiting for updater: waiting for container: context deadline exceeded" job_id=44344904

Getting this message at end of logs, on more than one occasion, please advise. We are on Dependabot (not preview) and have seen this on more than one occasion now so I figured I'd open an issue.

updater | time="2020-08-04T00:30:13Z" level=info msg="task complete" container_id=job-44344904-updater exit_code=137 job_id=44344904 step=updater
updater | time="2020-08-04T00:30:13Z" level=warning msg="timeout running job" error="waiting for updater: waiting for container: context deadline exceeded" job_id=44344904 

image

The link that says "learn more" links to https://docs.github.com/en/github/administering-a-repository/keeping-your-dependencies-updated-automatically.

Package manager/ecosystem

composer

Manifest contents prior to update

Updated dependency

What you expected to see, versus what you actually saw

Expect Dependabot not to timeout.

Images of the diff or a link to the PR, issue or logs

Forked On 16 Sep 2022 at 08:29:41

Driskell

The dry run I think wouldn’t help - composer bump is just a way of saying β€œthe lock has 1.2.1 but the requirement was ^1.0, increased the requirement to require what’s locked in so the minimum version is increased”

the dry run just outputs what would be changed in the requirements. No actual check for updates is done it’s just a way of telling composer AFTER an update β€œok so now you updated - now make this version the new minimum”. Tbh never been able to understand the need but hey lots of sue cases around these things!

Commented On 16 Sep 2022 at 08:29:41
Issue Comment

Driskell

Timeout: error="waiting for updater: waiting for container: context deadline exceeded" job_id=44344904

Getting this message at end of logs, on more than one occasion, please advise. We are on Dependabot (not preview) and have seen this on more than one occasion now so I figured I'd open an issue.

updater | time="2020-08-04T00:30:13Z" level=info msg="task complete" container_id=job-44344904-updater exit_code=137 job_id=44344904 step=updater
updater | time="2020-08-04T00:30:13Z" level=warning msg="timeout running job" error="waiting for updater: waiting for container: context deadline exceeded" job_id=44344904 

image

The link that says "learn more" links to https://docs.github.com/en/github/administering-a-repository/keeping-your-dependencies-updated-automatically.

Package manager/ecosystem

composer

Manifest contents prior to update

Updated dependency

What you expected to see, versus what you actually saw

Expect Dependabot not to timeout.

Images of the diff or a link to the PR, issue or logs

Forked On 15 Sep 2022 at 12:25:12

Driskell

  1. Do we not cache the output of the first composer update call? I would think each subsequent call would be quite quick. Or is the problem that we're using composer as a library and so it doesn't cache anything during its calls?

There's no caching dependabot side but I'm not sure what can be cached. Each update is separate and independent and so needs resolving on its own. Everything caching wise would be composer.

  1. I haven't read through the relevant source, but I assume we're doing the equivalent of composer update vendor/package and never do a broad-based composer update... but even in that case doesn't composer still under the covers have to run a composer update to ensure that there are no conflicts, and then post-network-fetch it filters the changes to only the specified vendor/package call? If it's somehow able to shortcut things, then we'd probably want to do a composer update at the front to pre-warm the cache...

Not quite the same as the library method allows specifying custom installers etc. so that Dependabot can prevent any writes or downloads of actual packages. Additionally, to use the CLI, because composer allows deep integrations like composer-patches extension and wikimedia-composer-merge extension it would immediately break because those would attempt to access repository data referenced in the composer.json, such as a patches file, custom extensions from the code base itself, or other composer.json files from downloaded and extracted packages. So you'd soon find you need to not only copy in the composer.json/composer.lock for the run but the entire repository as there's no way to tell what files will be accessed.

While it's not a near-term priority for us, it'd be nice to move from using composer as a library with native PHP helper code etc to instead using it as a CLI with various flags to extract the information we need... it lets us preserve the native package manager abstraction a lot better. We moved toward this in the go ecosystem, resulting in both speed and correctness improvements, not to maintain lower ongoing maintenance costs. So understanding the above questions would be helpful to understanding how much potential speedup we might get... for example it'd be a huge win if this changeover resulted from no cached calls to re-using a cache for most calls.

As noted the CLI doesn't let you stop downloads or installs. It's not intended nor do I think it ever will be implemented to allow the CLI to be run in a way that doesn't actually install or update, and only updates the lock files. So using the CLI will make things slower.

The only way I can think to make things quicker would be to, as you say, update everything at once. As that involves only one round trip to the repository then and one single resolution of package dependencies. That's a very different architecture to what dependabot uses currently.

I think the difference between the go ecosystem example and composer is the go one is extremely simple I think from what I remember of using it. Composer on the other hand is very advanced in its capabilities, with extensibility built in and various package management features like replaces and provides and conflicts and very advanced version constraint resolving, which I think tools like that in go don't come near to (probably because they know how complex it can get, and potentially slow in comparison)

Commented On 15 Sep 2022 at 12:25:12

Driskell

fix: Fix unpredictable false positive/negative of missing entity access check in function scope due to cache key conflict #475

Pushed On 15 Sep 2022 at 11:42:47

Driskell

fix: Fix unpredictable false positive/negative of missing entity access check in function scope due to cache key conflict

Pushed On 15 Sep 2022 at 11:42:10
Issue Comment

Driskell

Random false positive "Missing explicit access check on entity query"

I've been hitting some issues for a month or so where randomly after switching a branch I would see several "Missing explicit access check on entity query" reported. However, they're all fine and fixed. I would move the accessCheck() call around and then run again and the error would vanish. I would put the line back to the original and it wouldn't fail anymore it would be like a ghost that's now gone.

Push forward to now, and we upgraded PHPStan and went from 1.1.20 phpstan-drupal to 1.1.25 and now we have 6 false positives in our CI which won't go away. No matter what we do with the accessCheck call they report. And if we checkout the exact same code version and run PHPStan we get zero errors. So we seem to be seeing different results depending on where it runs - somewhat similar to before where random people would get random failures and it would just disappear - but instead it's now on the CI, no longer random, and completely reproducing every time. CI is GitHub Actions hosted so ephemeral. We just can't get the PHPStan to pass anymore. Locally, we get 0 errors and cannot get them to appear. I managed to get them once but they vanished on the second run, but no luck since.

Can't rule out something really weird in our configuration but I've yet to find it. The only difference I haven't explored in detail is GitHub Actions is using Ubuntu and I am using macOS.

It's only ever been a problem with this specific error though.

Forked On 15 Sep 2022 at 11:41:36

Driskell

OK fully reproduced. Had an issue where I couldn't see why it wasn't failing for class methods and noticed there's different cache key when the code is inside a class method. But that was a red herring and I was just not writing the test right clearly.

PR will contain 2 test cases, function and class method, with the cache key fix that works for me.

Commented On 15 Sep 2022 at 11:41:36

Driskell

fix: Fix unpredictable false positive/negative of missing entity access check in function scope due to cache key conflict

Pushed On 15 Sep 2022 at 11:28:08

Driskell

fix: Fix unpredictable false positive/negative of missing entity access check in function scope due to cache key conflict

Created On 15 Sep 2022 at 11:26:56
Create Branch
Driskell In driskell/phpstan-drupal Create Branchfix-entity-query-type-cache

Driskell

Extension for PHPStan to allow analysis of Drupal code.

On 15 Sep 2022 at 11:24:41

Driskell

Extension for PHPStan to allow analysis of Drupal code.

Forked On 15 Sep 2022 at 11:15:53
Issue Comment

Driskell

Random false positive "Missing explicit access check on entity query"

I've been hitting some issues for a month or so where randomly after switching a branch I would see several "Missing explicit access check on entity query" reported. However, they're all fine and fixed. I would move the accessCheck() call around and then run again and the error would vanish. I would put the line back to the original and it wouldn't fail anymore it would be like a ghost that's now gone.

Push forward to now, and we upgraded PHPStan and went from 1.1.20 phpstan-drupal to 1.1.25 and now we have 6 false positives in our CI which won't go away. No matter what we do with the accessCheck call they report. And if we checkout the exact same code version and run PHPStan we get zero errors. So we seem to be seeing different results depending on where it runs - somewhat similar to before where random people would get random failures and it would just disappear - but instead it's now on the CI, no longer random, and completely reproducing every time. CI is GitHub Actions hosted so ephemeral. We just can't get the PHPStan to pass anymore. Locally, we get 0 errors and cannot get them to appear. I managed to get them once but they vanished on the second run, but no luck since.

Can't rule out something really weird in our configuration but I've yet to find it. The only difference I haven't explored in detail is GitHub Actions is using Ubuntu and I am using macOS.

It's only ever been a problem with this specific error though.

Forked On 15 Sep 2022 at 10:46:26

Driskell

Found the issue.

https://github.com/mglaman/phpstan-drupal/blob/1.1.25/src/Type/EntityQuery/EntityQueryDynamicReturnTypeExtension.php#L59-L78

The return of $defaultReturnType is problematic - when tracing PHPStan this default return type comes out of a cache of the return type for the method being used. In my case, randomly, depending on file processing order, the cached result of this for a condition() call can sometimes return a cached EntityQueryType ancestor with hasAccessCheck false and because the Dynamic Return Extension doesn't run for this method it leaves it as the cached value.

The cache key for a return type appears to be purely based on method itself and not scope of the $this.

Will look at what needs to happen to involve the call chain in the cache key.

Commented On 15 Sep 2022 at 10:46:26
Issue Comment

Driskell

Random false positive "Missing explicit access check on entity query"

I've been hitting some issues for a month or so where randomly after switching a branch I would see several "Missing explicit access check on entity query" reported. However, they're all fine and fixed. I would move the accessCheck() call around and then run again and the error would vanish. I would put the line back to the original and it wouldn't fail anymore it would be like a ghost that's now gone.

Push forward to now, and we upgraded PHPStan and went from 1.1.20 phpstan-drupal to 1.1.25 and now we have 6 false positives in our CI which won't go away. No matter what we do with the accessCheck call they report. And if we checkout the exact same code version and run PHPStan we get zero errors. So we seem to be seeing different results depending on where it runs - somewhat similar to before where random people would get random failures and it would just disappear - but instead it's now on the CI, no longer random, and completely reproducing every time. CI is GitHub Actions hosted so ephemeral. We just can't get the PHPStan to pass anymore. Locally, we get 0 errors and cannot get them to appear. I managed to get them once but they vanished on the second run, but no luck since.

Can't rule out something really weird in our configuration but I've yet to find it. The only difference I haven't explored in detail is GitHub Actions is using Ubuntu and I am using macOS.

It's only ever been a problem with this specific error though.

Forked On 15 Sep 2022 at 08:36:15

Driskell

All instances are of the below pattern where condition appears after accessCheck. When accessCheck appears at the end just before execute the errors disappear even when running with --debug.

 $storage = \Drupal::entityTypeManager()->getStorage('entity');
  $ids = $storage->getQuery()
    ->accessCheck(false)
    ->condition('field', '', '<>')
    ->execute(); 

Commented On 15 Sep 2022 at 08:36:15
Issue Comment

Driskell

Random false positive "Missing explicit access check on entity query"

I've been hitting some issues for a month or so where randomly after switching a branch I would see several "Missing explicit access check on entity query" reported. However, they're all fine and fixed. I would move the accessCheck() call around and then run again and the error would vanish. I would put the line back to the original and it wouldn't fail anymore it would be like a ghost that's now gone.

Push forward to now, and we upgraded PHPStan and went from 1.1.20 phpstan-drupal to 1.1.25 and now we have 6 false positives in our CI which won't go away. No matter what we do with the accessCheck call they report. And if we checkout the exact same code version and run PHPStan we get zero errors. So we seem to be seeing different results depending on where it runs - somewhat similar to before where random people would get random failures and it would just disappear - but instead it's now on the CI, no longer random, and completely reproducing every time. CI is GitHub Actions hosted so ephemeral. We just can't get the PHPStan to pass anymore. Locally, we get 0 errors and cannot get them to appear. I managed to get them once but they vanished on the second run, but no luck since.

Can't rule out something really weird in our configuration but I've yet to find it. The only difference I haven't explored in detail is GitHub Actions is using Ubuntu and I am using macOS.

It's only ever been a problem with this specific error though.

Forked On 15 Sep 2022 at 08:09:37

Driskell

OK so I have managed to reproduce on my local but only with --debug:

XXX 9:05:17 XXX$ ./vendor/bin/phpstan analyze
Note: Using configuration file XXX/phpstan.neon.
 912/912 [β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“] 100%



 [OK] No errors


XXX 9:05:17 XXX$ ./vendor/bin/phpstan analyze
Note: Using configuration file XXX/phpstan.neon.
 912/912 [β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“β–“] 100%



 [OK] No errors


XXX 9:05:17 XXX$ ./vendor/bin/phpstan --debug analyze
Note: Using configuration file XXX/phpstan.neon.
---file-list---
 ------ ---------------------------------------------------------
  Line   XXX
 ------ ---------------------------------------------------------
  122    Missing explicit access check on entity query.
         πŸ’‘ See https://www.drupal.org/node/3201242
 ------ ---------------------------------------------------------

 ------ ---------------------------------------------------
  Line   XXX
 ------ ---------------------------------------------------
  492    Missing explicit access check on entity query.
         πŸ’‘ See https://www.drupal.org/node/3201242
  506    Missing explicit access check on entity query.
         πŸ’‘ See https://www.drupal.org/node/3201242
  1426   Missing explicit access check on entity query.
         πŸ’‘ See https://www.drupal.org/node/3201242
 ------ ---------------------------------------------------

 ------ ---------------------------------------------------------------
  Line   XXX
 ------ ---------------------------------------------------------------
  76     Missing explicit access check on entity query.
         πŸ’‘ See https://www.drupal.org/node/3201242
  119    Missing explicit access check on entity query.
         πŸ’‘ See https://www.drupal.org/node/3201242
 ------ ---------------------------------------------------------------

 ------ ------------------------------------------------------
  Line   XXX
 ------ ------------------------------------------------------
  534    Missing explicit access check on entity query.
         πŸ’‘ See https://www.drupal.org/node/3201242
 ------ ------------------------------------------------------


 [ERROR] Found 7 errors 

Subsequent analysis without --debug then reproduces, as expected due to caching. But if I remove the PHPStan cache folder and run without --debug there is then 0 errors, and continues to be on each run. Running again after with --debug the errors then appear again, presumedly as --debug is doing something differently (non-parallel?) and ignoring cache.

Commented On 15 Sep 2022 at 08:09:37
Issue Comment

Driskell

Timeout: error="waiting for updater: waiting for container: context deadline exceeded" job_id=44344904

Getting this message at end of logs, on more than one occasion, please advise. We are on Dependabot (not preview) and have seen this on more than one occasion now so I figured I'd open an issue.

updater | time="2020-08-04T00:30:13Z" level=info msg="task complete" container_id=job-44344904-updater exit_code=137 job_id=44344904 step=updater
updater | time="2020-08-04T00:30:13Z" level=warning msg="timeout running job" error="waiting for updater: waiting for container: context deadline exceeded" job_id=44344904 

image

The link that says "learn more" links to https://docs.github.com/en/github/administering-a-repository/keeping-your-dependencies-updated-automatically.

Package manager/ecosystem

composer

Manifest contents prior to update

Updated dependency

What you expected to see, versus what you actually saw

Expect Dependabot not to timeout.

Images of the diff or a link to the PR, issue or logs

Forked On 14 Sep 2022 at 09:06:20

Driskell

Drupal now removed the replaced declaration in 9.4.4 so I expect it to run much quicker as no 404 requests clogging the network per package. But I haven’t tested it.

We moved to only using dependabot for Drupal core packages and using Drupal itself to notify of module updates. Other composer packages tend to appear in Security tab when updates available. So I’ve been somewhat distanced from this issue

Commented On 14 Sep 2022 at 09:06:20
Issue Comment

Driskell

Timeout: error="waiting for updater: waiting for container: context deadline exceeded" job_id=44344904

Getting this message at end of logs, on more than one occasion, please advise. We are on Dependabot (not preview) and have seen this on more than one occasion now so I figured I'd open an issue.

updater | time="2020-08-04T00:30:13Z" level=info msg="task complete" container_id=job-44344904-updater exit_code=137 job_id=44344904 step=updater
updater | time="2020-08-04T00:30:13Z" level=warning msg="timeout running job" error="waiting for updater: waiting for container: context deadline exceeded" job_id=44344904 

image

The link that says "learn more" links to https://docs.github.com/en/github/administering-a-repository/keeping-your-dependencies-updated-automatically.

Package manager/ecosystem

composer

Manifest contents prior to update

Updated dependency

What you expected to see, versus what you actually saw

Expect Dependabot not to timeout.

Images of the diff or a link to the PR, issue or logs

Forked On 14 Sep 2022 at 09:04:10

Driskell

The cleared the cache and ran the update command with the profile flag, and it took 74.2 seconds, which is still faster than the 45 minutes timeout.

$ composer clearcache
All caches cleared.
$ composer update --profile
Memory usage: 71.78MiB (peak: 77.02MiB), time: 74.2s 

Worth noting if dependabot is checking for updates to many packages it will do an update per package. So for a composer with 10 packages all with newer versions available it will take 72 seconds for each so more than 720 seconds

Commented On 14 Sep 2022 at 09:04:10
Issue Comment

Driskell

RxJs 7 setting X-Requested-With by default when RxJs 6 did not

Describe the bug

Reraise of https://github.com/ReactiveX/rxjs/issues/6663

The X-Requested-With header is still getting set in v7 with default parameters to an AJAX call where with RxJs 6 it did not, constituting a breaking change when using with a server that does not support preflight requests but does correctly return an Access-Control-Allow-Origin.

I've enhanced the reproductions to try show it more clearly in the codepen.

Expected behavior

Default parameter AJAX call to a non-same-origin endpoint which does not support Preflight but does return the correct allow headers to succeed. In other words - do not send a custom header in the request.

Reproduction code

Working: 6.6.7: https://codepen.io/Driskell/pen/yLovOxK

Not working: 7.4.0: https://codepen.io/Driskell/pen/MWvQymO 7.5.2: https://codepen.io/Driskell/pen/PoJyRaz 7.5.4: https://codepen.io/Driskell/pen/jOaGbwX 7.5.6: https://codepen.io/Driskell/pen/poVyyLo 8.0.0-alpha.4: https://codepen.io/Driskell/pen/NWMNNYV

Reproduction URL

No response

Version

7.5.2

Environment

No response

Additional context

No response

Forked On 09 Sep 2022 at 08:50:08

Driskell

@benlesh Did you have any thoughts on the above?

Commented On 09 Sep 2022 at 08:50:08
Issue Comment

Driskell

RxJs 7 setting X-Requested-With by default when RxJs 6 did not

Describe the bug

Reraise of https://github.com/ReactiveX/rxjs/issues/6663

The X-Requested-With header is still getting set in v7 with default parameters to an AJAX call where with RxJs 6 it did not, constituting a breaking change when using with a server that does not support preflight requests but does correctly return an Access-Control-Allow-Origin.

I've enhanced the reproductions to try show it more clearly in the codepen.

Expected behavior

Default parameter AJAX call to a non-same-origin endpoint which does not support Preflight but does return the correct allow headers to succeed. In other words - do not send a custom header in the request.

Reproduction code

Working: 6.6.7: https://codepen.io/Driskell/pen/yLovOxK

Not working: 7.4.0: https://codepen.io/Driskell/pen/MWvQymO 7.5.2: https://codepen.io/Driskell/pen/PoJyRaz 7.5.4: https://codepen.io/Driskell/pen/jOaGbwX 7.5.6: https://codepen.io/Driskell/pen/poVyyLo 8.0.0-alpha.4: https://codepen.io/Driskell/pen/NWMNNYV

Reproduction URL

No response

Version

7.5.2

Environment

No response

Additional context

No response

Forked On 09 Sep 2022 at 08:36:31

Driskell

Still seeing the rogue header in latest 7 and the new 8 alpha

7.5.6: https://codepen.io/Driskell/pen/poVyyLo 8.0.0-alpha.4: https://codepen.io/Driskell/pen/NWMNNYV

Commented On 09 Sep 2022 at 08:36:31
Issue Comment

Driskell

Do not unnecessarily load replaced packages into the pool

I'm seeing slow performance with composer and Drupal. I thought it may be related to the Pool Optimiser PR not having much impact on Drupal installations (see https://github.com/composer/composer/pull/9261#issuecomment-703171395) but it might not be. To be specific, this is when performing updates. It heavily impacts dependabot.

To reproduce. Take the composer.json from https://github.com/drupal/recommended-project/blob/9.2.x/composer.json Then install composer install --profile Then install a module composer require drupal/media_entity_twitter:2.5.0 --profile Modify composer.json for media_entity_twitter to ^2.0 Then update that module composer update -W drupal/media_entity_twitter --profile (goes to 2.6.0 currently.)

The timings I get are:

$ composer --version
Composer version 2.0.7 2020-11-13 17:31:06
$ composer install --profile
[31.7MiB/31.07s] Memory usage: 31.66MiB (peak: 228.37MiB), time: 31.07s
$ composer require drupal/media_entity_twitter:2.5.0 --profile
[19.1MiB/3.14s] Memory usage: 19.07MiB (peak: 20.42MiB), time: 3.14s
$ nano composer.json
$ composer update -W drupal/media_entity_twitter --profile
[46.9MiB/25.46s] Memory usage: 46.86MiB (peak: 203.91MiB), time: 25.46s 

As you can see the update takes a long time. When using a composer version that outputs the network requests, you can see many many 404 like these:

[23.5MiB/4.08s] [404] https://packages.drupal.org/files/packages/8/p2/drupal/core-bridge.json
[23.5MiB/4.19s] [404] https://packages.drupal.org/files/packages/8/p2/drupal/core-front-matter.json
[23.5MiB/4.34s] [404] https://packages.drupal.org/files/packages/8/p2/drupal/core-diff.json
[23.5MiB/4.35s] [404] https://packages.drupal.org/files/packages/8/p2/drupal/core-php-storage.json
[23.6MiB/4.43s] [404] https://packages.drupal.org/files/packages/8/p2/drupal/core-discovery.json
[23.6MiB/4.52s] [404] https://packages.drupal.org/files/packages/8/p2/drupal/core-file-cache.json
[23.6MiB/4.58s] [404] https://packages.drupal.org/files/packages/8/p2/drupal/core-file-security.json
[23.6MiB/4.62s] [404] https://packages.drupal.org/files/packages/8/p2/drupal/core-filesystem.json
[23.6MiB/4.83s] [404] https://packages.drupal.org/files/packages/8/p2/drupal/core-http-foundation.json
[23.6MiB/4.87s] [404] https://packages.drupal.org/files/packages/8/p2/drupal/core-graph.json
[23.6MiB/4.95s] [404] https://packages.drupal.org/files/packages/8/p2/drupal/core-gettext.json
[23.6MiB/5.00s] [404] https://packages.drupal.org/files/packages/8/p2/drupal/core-proxy-builder.json
[23.7MiB/5.05s] [404] https://packages.drupal.org/files/packages/8/p2/drupal/core-render.json
[23.7MiB/5.06s] [404] https://packages.drupal.org/files/packages/8/p2/drupal/core-uuid.json
[23.7MiB/5.09s] [404] https://packages.drupal.org/files/packages/8/p2/drupal/core-plugin.json
[23.7MiB/5.29s] [404] https://packages.drupal.org/files/packages/8/p2/drupal/core-utility.json
[23.7MiB/5.29s] [404] https://packages.drupal.org/files/packages/8/p2/drupal/datetime.json
[23.7MiB/5.35s] [404] https://packages.drupal.org/files/packages/8/p2/drupal/core-transliteration.json 

This goes on for a while.

Essentially the drupal/core-recommended depends on drupal/core package which has a large replaces list. This list contains the name of every core module. Whether or not it should is left for discussion elsewhere (there are reasons to do so.) This list of replaced packages for the most part do not exist - for example drupal/taxonomy is, and has always been, a core module (I think). When you run an update it sees that media_entity_twitter depends on drupal/core and thus unlocks it as a non-root dependency and marks it for load, but it then proceeds to load all the replacements - even though no other package ever requires them.

So it seems somewhat unnecessary to be loading the replaced packages. If there is indeed a require to one of them somewhere, it would make sense. Maybe I'm missing something.

With this PR the 404s disappear and the timings for the update becomes as expected:

$ composer update -W drupal/media_entity_twitter --profile
[29.7MiB/5.84s] Memory usage: 29.66MiB (peak: 250.26MiB), time: 5.84s 

Savings of 20 seconds.

WDYT?

CC @Toflar after discussion in #9261 (Thanks!)

Forked On 07 Jul 2022 at 10:37:09

Driskell

@stof @naderman Regarding comment https://github.com/composer/composer/pull/9619#discussion_r760237931

I initially investigated this and found it was related to a different area of code: https://github.com/composer/composer/pull/9619#discussion_r776339911

As per comment https://github.com/composer/composer/pull/9619#discussion_r777023213 I raised PR #10410 with that test and also what I think is the fix for the test.

So I'm unsure if there's anything remaining outstanding except a closer scrutiny on the change in this PR, which as per the last comment above could be increased in scope to delete the entire section of code below without failing a single test. So I think the remaining questions are: Is this code that slows down composer needed? Is there a test that will fail without it?

https://github.com/composer/composer/blob/24ce1eddbdb4658e1d106a5d982951342f86f4d3/src/Composer/DependencyResolver/PoolBuilder.php#L468-L489

Commented On 07 Jul 2022 at 10:37:09