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

Added repro for a bug with passing commands in markup controls #1742

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

tomasherceg
Copy link
Member

We found a strange bug - probably caused by missing validation of delegates passed to DotVVM properties.

I've added a repro /FeatureSamples/MarkupControl/CommandAsPropertyPage but I cannot find the place where to fix it.

@exyi
Copy link
Member

exyi commented Nov 26, 2023

I fixed the error message, otherwise the behavior looks "fine" to me.

  1. Normally, it should definitely error out, since the Click command has type Func<string, bool, Task> - i.e. it requires arguments, and the button provides none.
  2. You call the command from a value binding, so it makes sense that evaluating the value binding will call the command. As far as I understand it, _control.Click.ToString() invokes the value binding, which invokes the command.

Using the value binding to call commands is definitely not recommended, commands should only be invoked from command or staticCommand bindings. staticCommand is preferred to call control properties, since it works for calling both command and staticCommands

@tomasherceg tomasherceg marked this pull request as ready for review January 1, 2024 15:49
@tomasherceg
Copy link
Member Author

The ToString() workaround in the debug mode probably works because of the bug in .NET - see dotnet/runtime#96385

exyi added a commit that referenced this pull request Jan 27, 2024
The interpreter is broken, it causes problems in #1742.
The bug is reported in dotnet, without any response yet:
dotnet/runtime#96385
exyi added a commit that referenced this pull request Jan 27, 2024
The interpreter is broken, it causes problems in #1742.
The bug is reported in dotnet, without any response yet:
dotnet/runtime#96385
exyi added a commit that referenced this pull request Jan 31, 2024
The interpreter is broken, it causes problems in #1742.
The bug is reported in dotnet, without any response yet:
dotnet/runtime#96385
@tomasherceg
Copy link
Member Author

I have to add tests here.

@tomasherceg tomasherceg force-pushed the fix/command-properties branch from b974849 to 6d676be Compare February 8, 2024 16:42
@tomasherceg
Copy link
Member Author

We need to add additional checks:

  1. If DotVVM property is Command/Action/Func or any delegate, we should only allow command/staticCommand bindings.
  2. If DotVVM control property value is delegate, throw a reasonable exception.
  3. Disable lambda conversion except for in commands:
    image

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 this pull request may close these issues.

2 participants