@codebymikey

codebymikey → created an issue.

The test-only job you linked ended green, which implies it is not doing what we expected. It has

Ooh, I wasn't actually testing for failures, more that it was now able to pick up the appropriate Test.php file as being changed. The diff between the commits doesn't seem to have any changes that'd trigger an actual failure, hence why it passes.

We need to decide how to treat the various combinations of input of the two variables, because running the job when target branch and baseline SHA are identical is just confusing.

Yup, most users shouldn't use the hash reference as the _TEST_ONLY_TARGET_BRANCH, which is why it initially defaulted to the CI_DEFAULT_BRANCH.

I just erroneously did it as a quick test since 520125b0b wasn't the head of a referenceable branch name and was curious how it'd work.

I think it should work as was originally planned:
1. _TEST_ONLY_TARGET_BRANCH - should be a branch (or a commit hash - it should work, but is not officially supported)
2. _TEST_ONLY_BASELINE - should be an optional commit hash. If it's not declared, it should attempt to resolve it from _TEST_ONLY_TARGET_BRANCH which should be a git ref that shares a common history as the current one.

I have a plan to pass in a 'this is d7' argument to the main script, and then do away with the -d7 file altogether.

That sounds like a good idea!

The composer.json is modified at runtime, and that is the only change detected. So more investigation is needed. Any ideas?

It was because the original git ls-remote --sort="v:refname" origin "$TARGET_BRANCH" | tail -n1 | cut -f 1 call was written to pick up branch/tag references, but wasn't made to handle scenarios where the _TEST_ONLY_TARGET_BRANCH is set to a commit hash.

I've pushed an update to cater for it. The updated pipeline's available here.

codebymikey → created an issue.

Based off the documentation

Using compare_to with merged results pipelines can cause unexpected results, because the comparison base is an internal commit that GitLab creates.

It appears due to how Gitlab checks out the repo, the compare only works for clean commits where the commit hash it's comparing to is within its commit history.

In this case:
1. 3523139-test-only-changes -> 0603fba1 gives you a valid test-only changes pipeline.
2. The latest commit on the d9-basic branch (3cbc82f1) doesn't exist on the 3523139-test-only-changes branch, so it's unable to provide you with a test-only change diff for it.

Was able to trigger it on the 3523139-test-only-changes branch with:

_GITLAB_TEMPLATES_REPO: issue/gitlab_templates-3539542
_GITLAB_TEMPLATES_REF: 3539542-test-only-part-2
# The test-only pipeline isn't available because the branches need rebasing so that they share similar commit references.
#_TEST_ONLY_TARGET_BRANCH: d9-basic
# This works because the commit reference exists relative to the current branch.
_TEST_ONLY_TARGET_BRANCH: 0603fba103772e05c2c1357e55af2186aaeb4bd6
# Mainly for debugging purposes.
CI_DEBUG_TRACE: true

tldr; due to how Gitlab internally checks out the code, the branch the pipeline is running from needs to have the target branch's commit ref somewhere in its commit history, so a rebase is most likely needed.

We could make a temporary change to this projects .gitlab-ci.yml page rules so that the job is run in this MR, for verification before merging.

That's a good idea, and I've made the changes so it's a manual pipeline for issue forks. I think it's probably worth keeping for testing future changes to the docs.

Sample doc page with relative images running on the issue fork is available here.

Not a blocker, but how will this impact local testing?

That was actually my initial test of the new variable, and it worked well running under localhost.

The updated trim filter has been added.

The test-only patch pipeline also showcases the behaviour.

codebymikey → created an issue.

This would allow the 'test only' job to be run in pipeines on non-default branches, when triggered by other means

Yup! That seems like a great idea actually.

It is a bit of a fuss, and we have tried various way to get round it, but it is not possible.

Yeah, it does seem like it. I think from memory/personal experience with gitlab, and based on how the current variable precedence work, the best way to work around this is to lazily load the environment variables (similar to the .calculate-gitlab-ref) such that:
1. The variable isn't set on the Project/Group variable since that takes precedence over whatever the project has in its .gitlab-ci.yml file.
2. The _GITLAB_TEMPLATES_REPO and _CURL_TEMPLATES_REF are left empty in the main template, but the documentation references what the default value actually is.
3. The _GITLAB_TEMPLATES_REPO and _CURL_TEMPLATES_REF variables are lazily exported in the gitlab-ci.yml script and set to the default value when empty (that way, the custom overrides should take precedence over the "default" empty value).

Switching this to Needs Work so that it's not automatically closed as Fixed.

The fallback option seems useful, the more flexibility the better.

I'd still personally recommend that users avoid using strtotime() blindly since as per the original issue, it can accidentally convert it into an invalid date, so your feed should ideally have a standardized format to convert from (however having the fallback option still wouldn't hurt for cases where the dates are entered manually).

All that being said, the code and tests looks good! RTBC.

As per the discussion in ✨ Allow users to specify custom files that Test-only changes Active , I've created an issue/MR for loading the relative markdown assets.

codebymikey → created an issue.

This feature is available in the recently_read → and message → modules.

codebymikey → created an issue.

megachriz → credited codebymikey → .

I wasn't aware that you could actually modify the manual pipeline variables before running them, so initially worked on supporting test-only changes via the web pipeline.

This can be tested on the https://git.drupalcode.org/issue/stage_file_proxy-3521812/-/pipelines/new issue fork.

Test case 1: Merge request
Previous behaviour: Tests can't be ran
New behaviour: Test runs and fails gracefully.

Test case 2: Manual pipeline with no tests
Branch: 3.1.x
Variables:

_GITLAB_TEMPLATES_REPO: issue/gitlab_templates-3539542
_GITLAB_TEMPLATES_REF: 3539542-test-only-patterns
_TEST_ONLY_FILE_PATTERN: stage_file_proxy\.services\.yml
CI_DEBUG_TRACE: true

Result - doesn't create the "Test-only changes" job, because there are no test file changes.

Test case 2: Manual pipeline with tests
Branch: 3521812-test-web-pipeline
Variables:

_GITLAB_TEMPLATES_REPO: issue/gitlab_templates-3539542
_GITLAB_TEMPLATES_REF: 3539542-test-only-patterns
_TEST_ONLY_FILE_PATTERN: stage_file_proxy\.services\.yml
CI_DEBUG_TRACE: true

Result - creates the "Test-only changes" job since there are actual test file changes before the current branch and the default one.

On a sidenote, even though the 3521812-test-web-pipeline branch has the _GITLAB_TEMPLATES_REPO and _GITLAB_TEMPLATES_REF variables overridden and hardcoded in the .gitlab-ci.yml, new manual pipelines which don't explicitly specify the override end up using:

_GITLAB_TEMPLATES_REPO: project/gitlab_templates
_GITLAB_TEMPLATES_REF: default-ref

for some reason as can be seen here and here.

Yeah sure, no problem.

codebymikey → created an issue.

I was weighing the need for it, the main advantage of the additional variable is that it makes it a bit easier to add specific patterns to your project/test while also leveraging the default global pattern set on the template.

Personally would find it easier to simply add a dedicated stage_file_proxy\.services\.yml value to the pipeline variable, and have it be a bit more obvious as to why its being included rather than copying and updating the default variables from upstream.

However I don't have a strong opinion either way as its an edge case variable and will be an improvement over the current behaviour.

codebymikey → created an issue.

I don't think this issue has been fixed and probably needs further review.

The test-only CI pipeline fails to run because it requires the new stage_file_proxy.http_client service to decorate.

The following change on 2.0.x and 3.0.x causes the books to be rendered as a flat list on both of the Child order and Book routes.

-      $supported_depth = $route_name === 'book.node_child_ordering' ? max($depth, 2) : 2;
+      $supported_depth = $route_name === 'book.node_child_ordering' ? max($depth, 9) : 9;
       if (isset($data['link']['depth']) && $data['link']['depth'] > $supported_depth) {
         $indentation = [

This is because the $data['link']['depth'] > $supported_depth condition below it is never true.

On further investigation, the route checking logic isn't necessary, and the snippet could simply be replaced with:

-      $supported_depth = $route_name === 'book.node_child_ordering' ? max($depth, 9) : 9;
+      $supported_depth = max($depth + 1, 2);
       if (isset($data['link']['depth']) && $data['link']['depth'] > $supported_depth) {

And the indentation should work properly on all routes, unless the book outline route strongly needs to keep the indentation consistent with the top level book.

codebymikey → created an issue.

Instead of having a settings.php entry, how about a submodule, e.g. "Basic Auth with Site Lock" (the name probably needs work) - this would act as the flag instead?

The feature flag module idea is interesting, but seems like potential overhead (haven't benchmarked how expensive calls to the module handler checks are) for a feature that most sites would probably rarely need.

I took inspiration with how the phpinfo() status page behaviour → was changed, and the setting seemed like the way to go for a functionality tightly coupled with the site's security.

However if that's the convention going forward for core to make it easier for non-technical people to work with it, then I have no objection.

Would need input from members on the core team to provide further direction on this.

Added an update that ensures the category loading script behaviour is more consistent.

The nested rendering issue should be addressed by 🐛 Child ordering route renders nested books weirdly Active

The child books will always have at least a single indentation, but didn't want to complicate the logic a bit more than necessary to remove it, but feel free to tweak accordingly if the flat behaviour is desired.

codebymikey → created an issue.

Unsure why 2.0.x MRs are failing, but the failed tests are a separate issue as the tests pass locally.

Can confirm the baseline 2.0.x test failure behaviour with https://git.drupalcode.org/project/book/-/merge_requests/105

Created MR with test cases to showcase the behaviour.

Can't seem to trigger test-only pipelines on 3.0.x, but the test should fail if triggered.

Also fixed an issue with 3.0.x still referencing $node->book rather than $node->getBook(), as I needed it to ensure the tests would pass on the node/{node}/child-ordering route.

codebymikey → created an issue.

The module's already been updated to support Drupal 11.

Thanks for helping out with this!

codebymikey → made their first commit to this issue’s fork.

Thanks for all the work done on this guy!

> The basic auth provider will only act upon routes which have been explicitly tagged with the basic_auth _auth option.

If basic_auth is enabled, and basic auth is received on a route that is *not* tagged for basic auth, should we log or display a warning message?

That's an accidental entry to the issue summary, and actually meant for the 🐛 Basic auth returns 403 when username & password supplied but not needed. Needs work issue.

The changes so far only check for the username's existence.

But no, I don't think it's worth logging it as modules making use of basic_auth should already be tagged as such, and bad actors can use this as a means of filling up the logs without suffering the penalty of being banned by the flood control system.

Added the username check on the basic auth prompt (hidden behind a setting) and a simple test case for it.

codebymikey → changed the visibility of the branch 11.x to hidden.

codebymikey → made their first commit to this issue’s fork.

Updated issue summary with plans to move forward with this.

codebymikey → made their first commit to this issue’s fork.

codebymikey → created an issue.

Attached patch to apply for 2.0.x

The default MR keeps truncation on as part of the post update hook.

Here's a patch that can be applied on top of it that changes the default behaviour to having truncation off by default.

codebymikey → created an issue.

codebymikey → created an issue.

Reopened with a test case triggering the field level violation.

The test-only changes pipeline highlights the issue.

codebymikey → changed the visibility of the branch feature/3295745-multiple-connectors-5.0.22 to hidden.

megachriz → credited codebymikey → .

One thing I forgot to address with this was to ensure the template in final YAML config doesn't have \r\n characters → since that causes the template to be exported as a single line rather than a more human-friendly format. This should probably be addressed in a new issue since it probably affects the rewrite plugin too.

$this->setConfiguration([
  static::SETTING_TEMPLATE => str_replace(["\r\n", "\r"], "\n", $form_state->getValue(static::SETTING_TEMPLATE)),
]);

Possible improvement for a follow-up: allow arrays as replacement patterns.

And that's an interesting scenario, this is untested, but that's the beauty of now having Twig available, that scenario should be something users can potentially solve by composing multiple tamper plugins together to achieve the desired effect:
1. Have something similar to the Migrate Plus Transpose plugin to explode then map "name" and "extension" into a single array, then work with it that way (Optional) - On a side note, I think most of the process plugins from the Migrate module are useful inspirations for the Tamper module.
2. Twig plugin: create a multiline string of the items.

{# This assumes names and extensions are always the same length #}
{% set names = name|split('|') %}
{% set extensions = extension|split('|') %}
{% for i, name in names %}
{{ name }}.{{ extensions[i] }}
{% endfor %}

3. Explode plugin: explode items by new line into multiple values per row.

If it's not possible at the moment, then we'd just need to introduce a new plugin that makes it possible to achieve that.

Updated the merge request with the relevant tests highlighted in #6 as well as addressed most of the deprecation and cspell warnings with the exception of the ZfExtensionManagerSfContainer.

codebymikey → made their first commit to this issue’s fork.

Extended the work done by @dkosbob and created an issue fork.

Repurposed this issue for something else as it's a duplicate of an existing issue.

codebymikey → created an issue.

codebymikey → made their first commit to this issue’s fork.

codebymikey → created an issue.

I think having a granular permission makes sense in this case. Mainly for the reason that the administer node permission is typically used in relation to regular node entity operations, and flagging that the user is allowed to make those changes to the node without needing a specific permission for the bundle or field.

My current use case is for a "site admin" role that can essentially do most of the same actions as a regular admin as far as nodes are concerned (apart from rebuilding access), such as view and edit nodes of all bundle types without needing to be explicitly granted permission every time a new one is introduced, the current administer nodes is also currently used for other functionality like views and potentially in other contrib modules, that make use of the permission, so trying to move to the contrib solution probably wouldn't work for me here.

The rebuilding node action seems like a special operation that doesn't necessarily need to be linked to administer node permission, same as the current bypass node access permission.

Provided a new configuration option for specifying HTTP headers that may be specified on the Feed type or individual Feed entity level.

It seems like the latest commit doesn't work for my use case. I've updated the issue summary with steps to replicate the issue.

Unfortunately don't think I'll be able to test and work on this further as I'll be working on migrating to Klaro next, I just thought I'd document this in here because the next version is released.

codebymikey → created an issue.

codebymikey → created an issue.

No thanks, I'll push an issue fork/MR for it in due course.

codebymikey → created an issue.

codebymikey → made their first commit to this issue’s fork.

codebymikey → made their first commit to this issue’s fork.

codebymikey → created an issue.

codebymikey → made their first commit to this issue’s fork.

This appears to be a duplicate of ✨ Update CI configuration Active

codebymikey → created an issue.

codebymikey → created an issue.

Can't delete that bit of code, thats responsible for weeding out any scripts that won't be loaded on the page.

Is that really the case? From my tests and inspecting the code, the $disabled_javascripts entries are weeded out here, in particular: unset($javascript[$script]);, which is what the array_filter() was doing anyway.

I'm working on a solution at the js script end presently to fix it.

That's good to know, hopefully it's something similar to Klaro, where the script tags are replaced with a placeholder one that gets evaluated as a script when the user accepts cookies:

<script type="text/plain"
    data-type="text/javascript"
    data-eucc-category="analytics"
    data-eucc-attach-behavior="customModule"
    data-name="custom_module"
    data-src="/modules/custom/custom_module/js/custom.js">
</script>

codebymikey → created an issue.