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

Support Contao mono repo #214

Merged
merged 2 commits into from
Nov 29, 2023
Merged

Support Contao mono repo #214

merged 2 commits into from
Nov 29, 2023

Conversation

fritzmg
Copy link
Collaborator

@fritzmg fritzmg commented Nov 28, 2023

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.

@fritzmg fritzmg added the bug label Nov 28, 2023
@fritzmg fritzmg self-assigned this Nov 28, 2023
@aschempp
Copy link
Collaborator

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

@aschempp
Copy link
Collaborator

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

@Toflar
Copy link
Collaborator

Toflar commented Nov 29, 2023

Why would Composer care about how you build your monorepos?

@aschempp
Copy link
Collaborator

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
Copy link
Collaborator

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

@Toflar
Copy link
Collaborator

Toflar commented Nov 29, 2023

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

@fritzmg
Copy link
Collaborator Author

fritzmg commented Nov 29, 2023

(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
Copy link
Collaborator

Toflar commented Nov 29, 2023

There will be no version.

@fritzmg
Copy link
Collaborator Author

fritzmg commented Nov 29, 2023

True. I think we can change the check altogether.

@fritzmg fritzmg marked this pull request as draft November 29, 2023 08:57
@aschempp
Copy link
Collaborator

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
Copy link
Collaborator Author

fritzmg commented Nov 29, 2023

@qzminski the check was added in 25b2f0e - but what is the reason behind it? Shouldn't this also apply in Contao 4.13?

@qzminski
Copy link
Member

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
Copy link
Collaborator Author

fritzmg commented Nov 29, 2023

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

// Support file uploads in Contao 5
if (version_compare(InstalledVersions::getVersion('contao/core-bundle'), '5.0', '>=') && $widget instanceof UploadableWidgetInterface) {
return $widget->value;
}
if (!$widget->submitInput()) {
// Do not throw exception here for BC
return null;
}
return $widget->value;
}

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

@qzminski
Copy link
Member

qzminski commented Nov 29, 2023

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
Copy link
Collaborator Author

fritzmg commented Nov 29, 2023

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

if ($widget instanceof UploadableWidgetInterface) { 
    return $widget->value; 
} 
  • This will return null in Contao 4.13, since the FormFileUpload widget does not hold the value.
  • It will return the value in Contao 5

@qzminski
Copy link
Member

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

@Toflar
Copy link
Collaborator

Toflar commented Nov 29, 2023

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

@qzminski qzminski marked this pull request as ready for review November 29, 2023 11:11
@qzminski qzminski merged commit c211255 into main Nov 29, 2023
1 check passed
@qzminski qzminski deleted the hotfix/mono-repo branch November 29, 2023 11:12
@qzminski
Copy link
Member

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

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

Successfully merging this pull request may close these issues.

4 participants