Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Array of tasks in after/before are reversed #3365

Closed
erropix opened this issue Nov 1, 2022 · 7 comments · Fixed by #3400
Closed

Array of tasks in after/before are reversed #3365

erropix opened this issue Nov 1, 2022 · 7 comments · Fixed by #3400

Comments

@erropix
Copy link

erropix commented Nov 1, 2022

I am not sure what the reason the tasks list is reversed on before/after hooks:

protected static function after(array $after)
{
foreach ($after as $key => $value) {
if (is_array($value)) {
foreach (array_reverse($value) as $v) {
after($key, $v);
}
} else {
after($key, $value);
}
}
}

This makes no sense as we have to write the tasks in the opposite order of their execution.

@antonmedv
Copy link
Member

Don’t remember) we can remove it. I guess.

@erropix
Copy link
Author

erropix commented Nov 1, 2022

I just found out it was added in v7 1d137ad but the commit description doesn't mention anything about the tasks bein reversed and this behavior is not documented either

@erropix
Copy link
Author

erropix commented Nov 3, 2022

@antonmedv any update on this?

@hwmaier
Copy link

hwmaier commented Dec 11, 2022

I found this behaviour unexpected and a bit confusing. At least it should be documented, but I believe even better the order should be changed to what most people would expect: top of the list first, bottom last.

Schrank added a commit that referenced this issue Dec 11, 2022
In before and after the order of execution was reversed. This is counter intuitiv, therefore we change it.
@Schrank Schrank mentioned this issue Dec 11, 2022
5 tasks
@Schrank
Copy link
Contributor

Schrank commented Dec 11, 2022

At you all: Feel free to provide a PR yourself, especially for so easy tasks like this.

Now 🤞 the CI is green :-)

antonmedv pushed a commit that referenced this issue Dec 11, 2022
In before and after the order of execution was reversed. This is counter intuitiv, therefore we change it.
antonmedv added a commit that referenced this issue Jan 9, 2023
This reverts commit 37142c6.
midweste pushed a commit to midweste/deployer that referenced this issue Jan 27, 2023
* origin/master: (33 commits)
  Update default cachetool version to 9.0.0 (PHP 8.1 compatible) (deployphp#3462)
  Update UPGRADE.md (deployphp#3458)
  update writable deploy recipe to use release_or_current_path (deployphp#3449)
  Fix PostgreSQL provisioning
  Fix PostgresDB user creation
  Show failed tasks for CI-environments (deployphp#3446)
  Fix NodeJS 12 warning: update EndBug/add-and-commit action (deployphp#3442)
  Replace unmaintained satackey/push-prebuilt-action by a maintained fork (deployphp#3437)
  Fix Node.js 12 actions are deprecated (deployphp#3435)
  Fix set-output command is deprecated and will be disabled soon (deployphp#3436)
  Add "new version" banners on release
  Revert "Fixes deployphp#3365 (deployphp#3400)"
  Make Deployer work with PHP 8.2 (deployphp#3422)
  Update symfony/console to ^5.4.9
  Update symfony/console to ^5.4.9
  Test on php 8.2 as well
  Revert "Composer update"
  Composer update
  [automatic] Update docs with bin/docgen
  Feature/magento2 artifact deployment (deployphp#3317)
  ...
@erropix
Copy link
Author

erropix commented May 29, 2023

@antonmedv I guess the YAML array order for me is different than what's shown in #3431

Any idea what may cause this strange order difference?

@SimJoSt
Copy link
Contributor

SimJoSt commented Mar 24, 2024

I can confirm, that the order of tasks via the after hook is still an issue for me as well. before executes the tasks in the order they appear in the yaml file.
As a breaking change, it would probably need to go into a major release.

@antonmedv can you reopen this issue, so we can track this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants