Magento Architectural Discussions, 3 Apr 2019

Previous Meeting Next Meeting

⏯

youtube image

►

From YouTube: Magento Architectural Discussion -- April, 3, 2019

Description

- Code style rules
- Backward compatibility: adding optional argument in constructor

https://github.com/magento/architecture/issues/137

A

Yes, that's a factual meeting.

B

B

Do you see my screen? Yes, so.

B

Once Magento coding standard was delivered to my gender, we faced with some issues and there was some minor bugs discovered and there was a need to create new ADIZ. So the question about versioning appeared: that's why I created this document and described.

B

Some specific use cases related to coding standard and the peoples of how we will have, however, will have the releases. So the main idea is like to define what we like. What kind of changes can we.

B

Like can we assume that it was like by changes minor or major and I want to discard this question with you, for example, what effect for bug fix to exist in rule that just prevent false positive behavior? It's definitely a bite release, for example. What if we remove the road from the rule set? What if this is Magento written by us rule or or if it's out of box, who that is the part of PHP code, sniffer I assume this is also a patch release about universe.

B

That's that's a bachelor's about minor changes. This happens in case if we want to add noodle to the coding standard or like, if you want to add a generic flow or really want to implement some magenta specific and at this roti they're all set and break and change I assume it's related to namespace changes. If, for example, if you want to change the name of, it is right now.

C

B

And we want to change to something like Magento.

B

This years, or something like that or if we change but for requirements, for example with require instead of PHP, could see 43.1, we started by 4.0, etc. I need your thoughts here, because I'm not sure about the biggest black concern is removing those.

B

Can we assume that it's.

B

Yeah there was some, let's say, unexpected behavior one of the outer box. If that was not written by us, that was peach, P shells, generic sniff, that was trying to detect the function, doesn't contain, throw annotation in case if it can throw an exception of people and the unexpected behavior was that it doesn't work with inherited documentation.

B

So it doesn't take inherit dock into account. So what our only like one way to fix that was to remove this outer box rule from the file.

A

A

So I understand how we base our decision, whether its watch, minor or major, is.

A

If somebody just uses our our sniffs.

A

Sometimes we'll start filing on the same code base crap.

D

A

Yeah, we're done at least from what I see from the rules.

B

Yes, yes, that's the main point, so it gives. If we do, some checks loses its bite release if you want to enforce and make our Career Center stricter its minor of this, because in the that case, people need to do it's their whole.

E

Yeah, in my opinion, I think we should add, lose from standard to minor release, because if, if I or my team asking me hey, which ruse in the current coding standard and I can tell ya, this are boo-boos. And this is how the terrifying, and if we removing, who is from a patch or by a rough patch, I I, think it should be a minor. Because then the team has the possibility to commit on a minor version or not on a patch but I think removing is not the best way on the patch.

B

Removing magenta specific or removing any kind of like even out of box there I didn't get that point. I.

F

A

A

And again we are talking about.

A

And in Magento itself,.

A

B

Specified this in composer, the JSON file. Yes, yes,.

A

B

Yeah on patch right now it's on patch.

A

So I don't know, I would move removing.

A

G

G

And somebody extend this, it makes sense that we cannot just remove our names.

G

The same time, if you just it means that I.

A

G

Somebody extends from Europe and you you sniff and you don't move sleep, not remove different rules, but then we'd sleep is this case. It probably was imagine.

E

What what I mean is codes, never get less strict and a team has a commitment like we.

E

We try to implement this download or something like that and magenta removing this from the standard rule set, because there is not some definition in the in the coding guidelines. Maybe the team don't see? Oh there's a rule removed, but our team has committed to this rule. This is why I think it should be a minor release, not a patch release, because if, if you are using a standard in a team you're trying to have an alignment with your development team and all body.

E

Polo drew is not trying to do their own stuff and we are committing us to a standard and then some default standard is removed from the roost at Exmouth yeah. Then we cannot follow any more of the stand-up, because some rule is missing. Maybe it's important for test development. This is my opinion.

E

Because most agency will use the.

B

Okay, I would like to ask you to read your comments.

A

B

Thank you and then.

A

C

A

B

I will go to the next topic, so we have open to requests to Magento coding standard. This is the implementation for the room that boxes and interceptors cannot be explicitly requested in constructors.

B

My question is: I would like you to show examples what we are trying to find. They are trying to find that a case, even if it's in the use- and it's explicitly requested.

B

In the constructor we throw we throw an error, so the most important question is: what type of severity is this? Can we reject extensions because of this so I believe the in case? If somebody decided to to make such mistake, it will like break the part of interception, functionality, approximation knowledge statement.

B

So yet and question is like severity, determines that in case, if somebody will make such a mistake, in extension, extension will be automatically rejected. This is an expected behavior.

C

Check it because proxy definitely will work enjoy computation and constructing normally is not problem. It's just recommendation to not work it, but for intercept and so.

B

So you believe that proxy can be explicitly refers. The difference yeah.

C

If it's definitely will work.

B

Okay, it will work, but it's not recommended yeah.

C

It's not recommended.

C

G

C

I don't know, but I never sees that someone tried to use interceptors I.

B

Run this check against Magento and there are a lot of findings.

C

B

Proxies and interceptors.

G

And that's why I mean if static test is really.

B

So you think it's high severity.

G

How severe wouldn't be for them a romantic would be if it starts failing. They have post a dependency or a sedative and I.

B

Missed your point: well, what Dana? Yes.

G

We should ask.

G

B

Do you mean the four coils not a problem? We.

G

Can fix it I think.

A

So what severity means.

A

Yes, in my opinion, it's really unreliable implementation. It might work today.

A

Practice I don't know, I would keep okay.

B

I will I will keep them discussed. This was extension developers, okay,.

B

And now I had two topics. Also two protocols recording standard large. They want to proceed or you want me to go over this.

E

B

I will start if you will have something for that.

B

So previously we agreed about the rule that slow rate function should like reemerged should not be used in look so here's the example record, and here is the example how to fix it.

B

Lloris implemented this room and when I run this against Magento, we found this amount of onions. It's 2:00 a.m.

B

The question is like: are we still up to do this and to accept this rule.

B

Again because in case you there will be fine the changed file, even if.

B

It will be in the I.

A

G

B

Just we just say that here is the Runyon message.

G

There is context show, and we should make sure that textual help gives instruction politics.

B

Will help it's like what safer we can just show the message we can add to the message.

F

G

In conventional message via message, if we add instructions message, then it will be obvious.

E

Currently, there are two points: first I found a park and phpstorm plugin if I implementing this stuff, because I was terrifying and some test cases and I implement a lot of test cases and this poor request.

E

That's I, think 20 test cases or something like that with different implementation or for each and for and data and gathers and then around this over the magenta coat and very fine, most issues manually and then I found a park in the year, a peach, pea, plugin and then I open with purposed for for the plugin, as also I changed my implementation and ran again and run again and run again and and now we have many findings. But some findings are not so easy to fix.

E

I do some fixing fixes for MSI result, I think we had two or three failing integration tasks, because some edge cases are not covered by the good example, because you can, if you have very complex erase, you can also do things wrong. It's not so easy to like. This is the pattern and you can refactoring like that. It works for 99%, but you have 1%.

E

Then you need our other way to refactor this code to a better way, and this is currently, if you have very complex erase, it can be very complex to refactor to a better performance way.

G

B

Was designed just.

G

B

F

We can use.

B

Vika, for that, like create separate page to was like some specific rules, was recommendations or describe all of the rules with the combinations.

B

Okay, I, don't think how to manage this, but anyway we we accept this and the last one also based on technical guidelines. The is that.

B

Who know tribal undetectable processing system resources?

B

The implementation was so in case if we filed generic PHP function. That starts with streams of it. Oh, we we want to see here. They wrapped with trycatch.

B

Just want to make sure this is the expected behavior.

A

B

Know we look for HP generic function. That starts with coral stream or socket, and we want to see them.

B

B

Yeah yeah, because you here, we definitely know that those functions work with system resources and not the point here is that it goes a little in conflict with discouraged functions because have a list function. We prefer not to use in magenta modules and our suggestion is to use framework wrappers, so I'd prefer to add here the line that we applied. This on.

B

C

B

I will add my comment regarding libraries, so I believe this should like to frame my situation.

B

Okay, I think that's it.

B

A

H

Is which functions really want to I mean which functions we specified ended.

B

With strike H no.

H

Not in the dress cage for functions for which we need to create four operas.

B

This is the list of discouraged functions.

B

What what is the question again.

H

So like we were just discussing sniff that check that for for a function that we use a proper instead of a function, I just wanted to see what are those functions? Okay,.

H

Okay, make sense. Okay, thank you. Thanks.

B

That's it from.

E

Question sorry and if extension developer, using stream or curl or something like that, what will happen they get warning, hey, don't use it. The please implement a wrapper or what I get because of some cases. In some cases you need. These functions were for some use cases of the extension. Maybe it's yeah, currently I, don't I, don't have a use case, but I think it can be possible to have a curl or open me to open connection to order system, and maybe the framework doesn't support option.

E

Of course, that memory was that timeout or something like that and for that reason I must implement by my own and if we only check the framework, then maybe the developer forget to try catch and run in an error currently and I'm, not so sure about a use case, but I think it can be possible.

A

So I think it can be managed by.

A

Some severity Prabhu still can accept extensions, but.

A

Is necessary in the future so like? This is something that we want.

A

E

Speak about a try-catch stuff, because if a diva extension developer forgot the try catch and their extension and using scream or something like that, then it can be. Maybe a problem. So.

A

You say that we should not.

E

This is what I mean, but then we have a problem that the magenta core and I I don't know how we kind of handle this problem.

E

Because of the discovered our functions also, maybe the extension developer cannot use a stream or something like that. Because then the extension will reject it of the usage of stream or something like that.

E

But I think we don't shoot a low of or we don't shoot or you should allow extension developer to use some functions to open connections. Maybe it can be at home but yeah in the most cases, it's correct to use the default framework stuff, but in some cases you have a use case. Maybe you cannot use the framework yeah.

A

Yeah, that's what I'm saying that for, but this is about discouraged, I.

H

Have a small question: how should we treat aging optional parameter in API class constructor as a as a patch minor change.

A

H

It's minor, but we like different. We were discussing this issue yesterday and have different opinions. So currently it's it's minor, but there is a proposal to treat that as a as a PI change.

I

I

A

A

A

For everything else, I think.

A

It can be for change because normally constructed is not part of.

A

G

J

H

We will be creating this rule, we will, we will be creating this list of classes. We will be very strict about classes in this list like if this is not some well-known layer supertype, then we would say that this class is not meant by extension. For instance, we have some session manager, magenta framework session session manager and some classes extent. There is this session manager and potentially another extension may want to extend the like this this class, but we would say that this is we.

H

We would not include this class since at least because we don't want this close to be extended. It doesn't look like it's a well-known supertype right.

H

How would we decide what what does a class is meant for extension? Gives us a well known we're super types only that.

G

Was my idea is okay, as usually I saw, you mentioned to be discouraged, inheritance and the only case when we.

G

Have to- and this is most employers who provides, because otherwise the cost just will not be worth. It is just too much work is our framework I.

A

E

Yeah magenta doesn't recommend inheritance, but I see so many extension. They use inheritance for API classes. They are not so known because there are some protected method in there or some public method in there or maybe a protective method and I need to extend this protective method. With my own code, it happens so many times, I see so often an extra or, in extension, I think yeah can break.

K

Something in some cases, but it should be fine unless you try to customize.

G

This argument is optional and if you just try to run, you should be okay, if you try to customize dependency injector, for example like what what can be. Let's assume we have currently identical through one and then in magenta.

G

Two three two, for example, introduce optional dependency in one of the classes and music developer, pass custom value or for that optional argument, and you expanded your logic, work somehow based on that customization and if your extension is installed on Magento, two, three one where there is no such functional argument, constructor customization of you will just work and on marketplace there is no way to specify compatibility on punch level version.

G

So if you release an extension, you will have to say that your extension is compatible is magenta 2.3 and obviously it might be broken depending on what version. But that only happens if you actually try to inject custom version of.

J

Version because.

J

Which, probably you want to customize so I? Think if like if you own extension, depends on the minor version you problems correct.

G

But you, the problem is, if you develop extension, let's say now: most likely you will develop it against later student 3 and you may just hold it.

G

But why do depends on two batteries, so you should not, but it's possible that your extension.

E

Maybe maybe there is no, not so many cases but I think it's still possible and there are so many extension developer, not know about best practices and Magento. Also yeah, the marketplace is really okay, but if you have a own marketplace or something like that and extension doesn't develop, doesn't know about a I, don't should use inheritance and adding own parameters to the constructor. It maybe breaks and I must check it. How often and in which cases it breaks but I think it can be possible and I.

E

Think if you have a patch release, you will check more. Your extension like if you have a minor.

E

Sorry, if you have a patch, you don't check so much your extension, maybe you install it and if you have a minor or major version, you check your extension more. This is my opinion, but I think yeah.

A

E

Defined in which cases it breaks and then have a proposal and see how many cases it will be or, and then, if we know that we can vote or up vote for this proposal because in in the last time some edge cases are not in mind of of the core team because they don't develop extension for customers and we have to fix the stuff or this extension for or air fix, the extension that the customer was buying and yeah sometimes I see some stuff.

E

That should not happen not developed by tech diversion developed by order develop but yeah. It can happen and we should define the cases, and this is my opinion and then we should vote or dole.

E

Don't accept this podcast.

C

Actually, from my opinion, if any changes emergencies break some scenario, so if you don't follow our best practice, don't fry test by your extension, any changes in Magento you can break can break your extension and if mine reason for to not accept this, that we can break time extension okay will always break some extension every spy she releases, because any changes can break some scenario and for function. If we talk about functional break, it be different to you. If you just return plus one in a next extension have to expect some and you break it.

C

But from what point we don't change anything yeah.

B

C

An extension is not point, I believe.

A

C

Extension for our rules, so, for example, if you depend on extend from EPA, quote just the depend on fudge version exact price fashion, but you know that your education can possible break this new patch version and you check your extension on every.

H

C

Write test and you will see breaking changes. You can provide fix, resist change.

A

Because also so mentioned, there are cases where extension developers expand from poor Plus just because they don't know about best practices, and there are cases were where it's just impossible. To avoid inheritance, I want to distinguish two situations.

A

C

Always can do not extend because if you need to extent it you can just cut the class in your extension and changing and polarity be a queue and resolve all issues will be on you.

H

Say that if you extend the pay a class, you can depend on major version.

H

E

I had in the past, I don't remember on the extension, but we had a patch. This was break in an extension of adding a constructor parameter or something like that because they don't use tasks. Yet the marketplace goes the way you should have to tests and more tests. It's are usable and get more practice or in Magento, two or more known, but yeah. Currently we don't have to state of any extension has test and any developer knows about the best practices of Magento and yeah.

E

Maybe it's the wrong way to don't allow changes to some methods because our we have not developer or we have developers. They are not known about best practices but I think yeah. It's it's not a way to break.

E

We are adding our.

E

A

A

What classes should be considered for which classes.

A

So, let's continue with.

H

The main reason to treat constructors as a private as a as a lock adn optional parameter the constructor as a patch. Is it it's a private API use.

A

A

E

Cool. Thank you.