codefog / contao-haste

Haste is a collection of tools and classes to ease working with Contao
http://codefog.pl/extension/haste.html
MIT License
43 stars 24 forks source link

Support Contao mono repo #214

Closed fritzmg closed 7 months ago

fritzmg commented 7 months ago

When using the Contao mono repo the following error might occur:

TypeError:
version_compare(): Argument #1 ($version1) must be of type string, null given

  at vendor\codefog\contao-haste\src\Form\Form.php:929
  at version_compare(null, '5.0', '>=')
     (vendor\codefog\contao-haste\src\Form\Form.php:929)

This PR fixes that by allowing to fall back to the mono repo if contao/core-bundle is not found.

aschempp commented 7 months ago

Shouldn't Composer return the version of a package that is provided by another package?

aschempp commented 7 months ago

I mean that sounds like something that should be fixed in Composer instead? 🤔 /cc @Toflar

Toflar commented 7 months ago

Why would Composer care about how you build your monorepos?

aschempp commented 7 months ago

It's not about monorepo. It's about the fact that we check for a version of a package, but the package has been replaced by another one. So I would expect to get the version of the replacing package.

aschempp commented 7 months ago

Or maybe there is a way to find the "final" package to check that version? 🤔

Toflar commented 7 months ago

No idea 🤷‍♂️ I think the PR looks correct (will still not work with dev branches but that's what it is).

fritzmg commented 7 months ago

(will still not work with dev branches but that's what it is)

The package name of a dev branch will still be contao/contao.

Toflar commented 7 months ago

There will be no version.

fritzmg commented 7 months ago

True. I think we can change the check altogether.

aschempp commented 7 months ago

No idea 🤷‍♂️ I think the PR looks correct (will still not work with dev branches but that's what it is).

This PR works, because the author of this extension assumes to have either of those packages installed. But if contao/core-bundle is replaced by foobar/core-bundle fork, it will not longer work either.

fritzmg commented 7 months ago

@qzminski the check was added in https://github.com/codefog/contao-haste/commit/25b2f0ec3d7d3199ceb3c43c1a3193a33d4eebfb - but what is the reason behind it? Shouldn't this also apply in Contao 4.13?

qzminski commented 7 months ago

The problem is with the file uploads. They have been reworked by @Toflar in Contao 5 and there is no longer $_SESSION['FILES'], but now they are returned as the widget's value. In Contao core it changed here: https://github.com/contao/contao/blob/5.x/core-bundle/contao/forms/Form.php#L273

fritzmg commented 7 months ago

Yes but the method returns $widget->value in any case:

https://github.com/codefog/contao-haste/blob/25b2f0ec3d7d3199ceb3c43c1a3193a33d4eebfb/src/Form/Form.php#L928-L939

$widget->submitInput() should return true (unless the widget is disabled or readonly).

qzminski commented 7 months ago

That's exactly the problem: https://github.com/contao/contao/blob/5.x/core-bundle/contao/forms/FormUpload.php

The upload field does not have the $blnSubmitInput = true.

fritzmg commented 7 months ago

Right, but then we can still remove the version check, can we not? Because:

if ($widget instanceof UploadableWidgetInterface) { 
    return $widget->value; 
} 
qzminski commented 7 months ago

Theoretically yes. I think that would be the appropriate fix. @Toflar what do you think?

Toflar commented 7 months ago

If it works, it's the appropriate fix :D

qzminski commented 7 months ago

I have just released it as 5.1.14, thank you @fritzmg !