Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mfernea created an issue. See original summary.

mfernea’s picture

My solution would be to add into composer.json:

  "require": {
    "drupal/core": ">=8.7.0"
  },
geek-merlin’s picture

Title: prepareDirectory vs file_prepare_directory » Breaks with core <8.7

...plus the same condition in .info.yml.

Patch much appreciated!

greggles’s picture

Title: Breaks with core <8.7 » prepareDirectory vs file_prepare_directory breaks with core <8.7

I think why it breaks is important ;)

jason_purdy’s picture

This just bit me. :( So I guess until this is addressed, those of us with Drupal 8.6, we need to pin stage_file_proxy to 1.0-beta1.

greggles’s picture

Like this?

geek-merlin’s picture

I think we should also add that .info.yml condition for the folks that don't use composer.

geek-merlin’s picture

Ah, also the patch contains a bogus comma, which makes it invalid json.

greggles’s picture

This composer should parse :)

I added the change to the info.yml but that is only supported since 8.7.7 so it doesn't really help for this situation, right?

greggles’s picture

I also added a warning to the project page.

mfernea’s picture

@jason_purdy Yes. At least this is what I did.
@greggles I think a message here https://www.drupal.org/project/stage_file_proxy/releases/8.x-1.0-rc1 would also help.

greggles’s picture

Good call, I've done that now.

mfernea’s picture

I noticed this in [#3070687].

The core_version_requirement cannot be used to restrict to core version before 8.7.7. For instance the following info.yml would throw a parsing exception:

name: My Module
type: module
core_version_requirement: ^8.7 || ^9

This is not valid because ^8.7 would include versions like 8.7.0 which do not recognize the core_version_requirement: key.

So we only need to alter the composer.json file.

mfernea’s picture

Status: Needs review » Needs work
greggles’s picture

Status: Needs work » Needs review
FileSize
657 bytes

Great catch.

Status: Needs review » Needs work

The last submitted patch, 15: 3084611_indicate_87_requirement-15.patch, failed testing. View results

greggles’s picture

Status: Needs work » Needs review

It says it "failed testing" but also the build details show "08:38:42 Finished: SUCCESS" on the jenkins console. I guess that was an issue b/c I changed the testing to focus on PHP 7.2 instead of php 5.6 (5.6 tests are failing now due to a composer issue and honestly php 5.6 is quite old at this point).

Status: Needs review » Needs work

The last submitted patch, 15: 3084611_indicate_87_requirement-15.patch, failed testing. View results

greggles’s picture

I didn't realize, but it seems there are no tests and when that's the case the build will be successful and yet the status gets moved to "needs work".

Here's an updated patch with a few code style fixes as well.

Anyone care to give a RTBC?

Status: Needs review » Needs work

The last submitted patch, 19: 3084611_indicate_87_requirement-19.patch, failed testing. View results

geek-merlin’s picture

I can confirm that the changes in composer.json and stage_file_proxy.info.yml make sense.

It looks like the rest is unrelated changes like whitespace fixes, which make sense but would suggest moving to separate commit.

I did not test that composer installs successfully (and afaik for composer reasons testbot/composer-patches is not the same).
But as such a test can only be done after a commit, let's do it and in the unlikely case we can fix that before a release.

geek-merlin’s picture

Spokje’s picture

- Patch #19 applies cleanly to the latest dev ('dev-1.x#77880c3dccf30160541a270c82d27672b5a5be26').
- Code Style fixes all make sense to me.
- Tested on Drupal 8.7.7 and all worked for me.

Just one small nitpick:

I've added the lines

dependencies:
  - drupal:system (>=8.7)

to stage_file_proxy.info.yml
This will prevent users from enabling the module in the GUI when on Core < 8.7.x (Drush will still install it).

Screenshot is from a D8.6.17 site, checkbox to enable is greyed out.
Only local images are allowed.

  • greggles committed b753a89 on 8.x-1.x
    Issue #3084611 by greggles, Spokje, mfernea, geek.merlin aka axel.rutz:...
greggles’s picture

Status: Needs work » Fixed

Thanks, @geek.merlin - all helpful!

Nice idea, @spokje!

Now committed/pushed.

I think we should make a new release as well in the very near future.

Spokje’s picture

Status: Fixed » Needs work

I think we should make a new release as well in the very near future.

+1 from me on that. :)

Spokje’s picture

Status: Needs work » Fixed

And a -3 for accidentally changing the status :(

greggles’s picture

greggles’s picture

In case anyone here is curious, this issue introduced a regression #3085394: 'core_version_requirement' issue with Core 8.7.8. Now fixed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.