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

com_dotnet extension: basic implementation (barely working) #845

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Indigo744
Copy link
Contributor

@Indigo744 Indigo744 commented Sep 3, 2020

Implements #834

I started to work on a basic implementation on extension com_dotnet

What works:

  • Creating COM object
    • codepage / typelib not supported
  • Calling methods with arguments which returns a simple value type (not COM objects)
  • Getting / Setting properties for value types (not COM objects)
  • com_create_guid()

What is missing:

Almost everything...! But the main work would revolve around implementing the Variant class so the COM class could inherit from it. Also this class would be used for handling complex COM object which is not working at all right now.

What should not be supported:

  • Creating dotnet object as .Net framework 4.0 and later are not supported.

@Indigo744
Copy link
Contributor Author

We should find a way to run the test only on Windows with specific extension activated? Not sure how to do this...

@jakubmisek
Copy link
Member

We should find a way to run the test only on Windows with specific extension activated? Not sure how to do this...

for now, at the beginning you can do something like

if (PHP_OS != "WINNT") exit("***SKIP***");

The test will be marked as skipped

@Indigo744
Copy link
Contributor Author

On the PeachPie side, should I add a test in the COM constructor (and in the function) where I test that we are on Windows and throw an NotSupportedException if not?

@jakubmisek
Copy link
Member

@Indigo744 I wouldn't check for Windows platform, what if it gets supported by .NET in the future (through magic) .. I guess .NET throws something anyways now

@Indigo744
Copy link
Contributor Author

Noted @jakubmisek

However, I'm not sure I'll be able to push this implementation further in the short term.

Would it be acceptable to merge it "as is"? Some stuff are working, so it would be better than nothing for people needing this. How could we warn about what is missing?

@jakubmisek
Copy link
Member

jakubmisek commented Sep 7, 2020

I guess tests :) and also before the release, we are generating the compatibility matrix on https://docs.peachpie.io/compatibility-status/ which you can generate for yourself at https://github.com/peachpiecompiler/peachpie-docs/tree/master/tools (it creates list of functions/classes in all loaded extensions - for PHP and PeachPie - and compares them)

@Indigo744
Copy link
Contributor Author

I guess tests :) and also before the release, we are generating the compatibility matrix on https://docs.peachpie.io/compatibility-status/ which you can generate for yourself at https://github.com/peachpiecompiler/peachpie-docs/tree/master/tools (it creates list of functions/classes in all loaded extensions - for PHP and PeachPie - and compares them)

I was able to generate a new updated list, but the com_dotnet extension sits at 0 / 107... I am missing the relevant package on the PeachPie side, but I couldn't get it to work, even with the 1.0.0-dev version..

@jakubmisek
Copy link
Member

you have to add a package/project reference into generate.msbuildproj

@Indigo744
Copy link
Contributor Author

@jakubmisek I was missing the lib from the local Nuget packages! Updating the script fixes this issue.

However, now I have the following error:

Unhandled exception. System.NotImplementedException: System.Reflection.BindingFlags
   at Pchp.Core.Dynamic.ConvertExpression.BindToValue(Expression expr)
   at Pchp.Core.Reflection.PhpPropertyInfo.ClrFieldProperty.CompileAccess(AccessMask access)
   at Pchp.Core.Reflection.PhpPropertyInfo.ClrFieldProperty.<.ctor>b__9_0()
   at System.Lazy`1.ViaFactory(LazyThreadSafetyMode mode)
   at System.Lazy`1.ExecutionAndPublication(LazyHelper executionAndPublication, Boolean useDefaultConstructor)
   at System.Lazy`1.CreateValue()
   at System.Lazy`1.get_Value()
   at Pchp.Core.Reflection.PhpPropertyInfo.ClrFieldProperty.GetValue(Context ctx, Object instance)
   at Pchp.Library.Reflection.ReflectionClass.getConstants(Context ctx)
   at CallSite.Target(Closure , CallSite , PhpValue , Context )
   at System.Dynamic.UpdateDelegates.UpdateAndExecute2[T0,T1,TRet](CallSite site, T0 arg0, T1 arg1)
   at <Root>.program_php.collect(Context <ctx>) in peachpie-docs\tools\generate-compatibility\program.php:line 45
   at <Root>.program_php.<Main>(Context <ctx>, PhpArray <locals>, Object this, RuntimeTypeHandle <self>) in peachpie-docs\tools\generate-compatibility\program.php:line 65
   at <Script>.Main(String[] args)

I guess it doesn't know how to handle the BindingFlags type related to the constant MemberAccessCom...

@jakubmisek
Copy link
Member

jakubmisek commented Sep 8, 2020

@Indigo744 this is because PHP reflection tries to deal with the following constant:

const BindingFlags MemberAccessCom

which should not be visible to PHP code at all. Annotate the constant with [PhpHidden] attribute so it won't get into PHP runtime at all (or mark it as internal protected).

EDIT: it should be private protected - this denotes a field or a method that is not visible in PHP context

jakubmisek added a commit that referenced this pull request Sep 8, 2020
@Indigo744
Copy link
Contributor Author

internal protected was not enough, I still had the error. Adding [PhpHidden] did the trick however!

@jakubmisek
Copy link
Member

@Indigo744 sorry, private protected hides the field/const/method from PHP reflection

@Indigo744
Copy link
Contributor Author

Do you want me to modify it further, or is it good enough for now?

@jakubmisek
Copy link
Member

Do you want me to modify it further, or is it good enough for now?

the attribute is ok for now

@jakubmisek
Copy link
Member

Hi @Indigo744 - can I help with this somehow? :)

@Indigo744
Copy link
Contributor Author

Hey @jakubmisek! Sorry for not getting back. I am swamped in work right now 😞 so as you could see I wasn't able to contribute as much as I could.

However note that this PR could be merged as is. Some stuff are working. The rest could be done later. As you wish 😸

@jakubmisek
Copy link
Member

@Indigo744 just checking :) would be great to have a list of feature that are not implemented so we would have an overview.

I'd be happy to implement whats missing eventually, and merge the PR

@Indigo744
Copy link
Contributor Author

I've outlined in the PR description what is defined/what is missing.

An exact list is available in the docs PR: peachpiecompiler/peachpie-docs#5.

@Indigo744
Copy link
Contributor Author

@jakubmisek Hello! Wow can't believe it's already 3 years later. Time flies!

Should we try merging it? I can clean up/fix conflicts.

@jakubmisek
Copy link
Member

@jakubmisek Hello! Wow can't believe it's already 3 years later. Time flies!

Should we try merging it? I can clean up/fix conflicts.

that would be awesome :)

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