Magento Architectural Discussions, 31 Jul 2019

Previous Meeting Next Meeting

⏯

youtube image

►

From YouTube: Magento Architectural Discussion -- July, 31, 2019

Description

- `@spi` annotation
- URL validator
- Composer performance

Meeting minutes: https://github.com/magento/architecture/issues/216

A

Okay, so this is a.

A

A

B

A

B

We have the description, but we don't. You can change any houses in our source code.

A

A

A

B

A

Don't encourage inheritance in the same.

B

Classes and we don't provide any other possibility.

A

My speech, yes, we say that we discourage inheritance, which means that by default nobody should extend classes and if nobody extends classes, then it means that all classes should be API not be considered as but for internal developers, it becomes impossible to modify those causes a and remove the message, because it is an idea and it will break somebody that you cannot add a message because it's considered SPI.

A

So, yes, we have some kind of God, it's not God classes, but there are some classes that we still like that I'd run. There is no other way other than inherit them, and we have this list here. What I don't like is that this list is in the documentation.

A

Somebody should remember or like an extension developer. You need to go, for example, to extend from this class. You need to go to the documentation and check with this class in the list, and you know that you can expand from it. If you extend another class, you also go to the Swiss and check that viscosity is not here.

A

So what I propose, instead of describing on all these rules in the documentation?

A

Just mark what it means that we will mark all these classes as SPI and as an API as well, and it will be more visible from the code itself, so the rules that I proposed and became first of all. What is a if there is something that you can call your application will be broken if there is an expansion. That was this class method.

A

Spi is something that you, for example, implement you extend behavior, so it can be implementation of the interface, for example, or additional plugins, so just to make sure that we are on the same page and the rules that I propose is. First, all the interfaces should be by default, Markus, API and because most likely remarkably type interface.

A

Because it can be can be different implementation for it all the interfaces should be able and display all the classes by default should be marked adjust and those special classes will be marked as SPI as well. So I also have I believe an example of how it will simplify the documentation.

A

Yes, simplification.

A

So on the Left, wouldn't is what we have.

A

More explanation about, while the table with all the rules is simplified, because there are some general rules that can be applied. It doesn't matter whether this is interface or class, for example, because.

A

In my opinion, you should simplify documentation, so there is, there are few fewer things to remember. Yeah.

B

Simplify the way.

B

There's currently all the existing interface supposed to be considered, that was.

A

B

So what we considered probably a year ago and the this question, so it may make sense to have the API API each.

B

Peer, the web only and whole other EPS, like all other interfaces, can be considered as a CPA. But if you do this, we can just apply that for the newly created quote, and if you do this, we have to echo which is smooth Amiens, so for because we can do it for legacy. For example, if you're talking about like product repositories, it's good example because there are too many examples of the product. Repository is used as an SPI and.

B

So it is substituted fully, for example, to provide the possibility of fusion like MongoDB, like storage engine or whatever or and along with that, we use it to like pretty widely at the API.

B

Two different approaches in the codebase and that will just.

B

C

B

The first approach just use, API API coverage and API demarcation for the API only and the one will set all the existing interfaces.

B

So we don't have the.

B

B

A

Probably can be ended for now.

A

And you add your commands to the proposal because I don't want us to.

D

A

We can discuss now and feedback.

E

B

As familles very difficult question to be Rosen and his community because, like for us, it's very important how those guys who work with that, whatever they are realized. What's a guy, an FBI from my experience, they make sure the.

F

Correctly and everything, experimentation to every other.

A

Interfaces classes.

C

I propose to do justice interfaces.

A

And those classes that we have as exceptions.

B

Regarding learning, the classes from my perspective is better to to discuss introduction of the final HP. The book that will, like the duel, better describes the idea that we don't encourage.

B

G

Ego and we don't you- can implement our di quad generations. Advise that it's resolved problem is final civic. You can not introduce final annotation but mark our classes. Like final, the technical.

A

A

Right, not the keyboard final, but actually.

G

Technical I don't remember this, who I discusses, but technically it possible to mark this final keyboard is require additional steps in process of code generation, but technically.

G

It's straightforward: we need remove final keywords and process of volume, but at runtime, so in code base it will the funnel but VanDerveer the load classes. We will remove final curve or tense. In this case, we can extend it.

F

F

Together some warden over these extensions and give them suggestions how to.

B

To check them, I mean for us should be found like more, not theoretical, but practical. So what you are saying it sounds for me that we will consider all the classes, except for those things as an API, so we are able to introduce.

H

So I think this is a additional validator that we need for a specific proposal that we had third party redirects, which didn't get to happen so we're just gonna use the base path, plus some some euro paths right and we don't have such a violator that in framework, so instead of this, is have nothing to do with graft you over any take Alice's shouldn't, be the framework and I need an approval for this, and it's all Basil's entering were friendly. They have this and basically you can turn on and off certain switches.

H

Like absolute paths, related paths, you can, how do they get parameters all that stuff? But this is specific about the URL route.

C

H

Rfc specifies it as route. Not path, then does refer it as path right. The Brabham is n is path so initially I called it path where the nicest route, because that's the heart of see through terminology into a URI, because you.

H

It's probably it probably is, and we can change the technology. So what I did I just copied to existing thing? That extends from zone by the abstract, and there were some I think that valid bit is, is wrong because there hasn't construct already a singleton that needs to be reconfigured here, like an absolute.

H

And like I wondering what should be done here like? Should we deprecate the other validator and come up with a better solution or should I just not touch framework? Maybe I need like a decision today, because.

H

Right I know this is very specific to something, but it's it's a pretty simple decision. You know I I'm, not proud of this. It's a simple copy from the other one, then that validated there should have been a factory. The constructor has a baby Toulouse in 2018.

H

It wasn't there and we tried to stay away from regex. We try to stay away from Parks's URL.

H

Basically, we have this graph cool stuff that you have to pass in a path where you're redirect from PayPal or some other payment system go when they go away from your site and they come back on post banks or whatever and that's a front-end knowledge right and think of it as a decoupled. You know knowledge before you. It was coupled because that knowledge came from the back end.

H

No, it doesn't so you need to make sure that that's half or route whatever you want to call it, and we we don't want to pass in a full URL, because that will become a security concern. So our first proposal was: we have a white list of domains that you can use and you will pass it for URL. The old validated will be good now, it's not because it was an improved and we trying to just make sure that it doesn't have directed traversal.

H

It doesn't have any given parameters and all that stuff, and you cannot do that with Magento. We didn't fully leverage this. Then you arrived validator possibilities and it's.

H

Right now you know I, don't even know why.

H

H

There's I think we have.

H

D

H

Avoid but that's what the other ear over there uses and it's used everywhere.

H

We kind of need our interface of how.

D

D

A

H

And they have some character allowance as well like what characters can use, because you can I, don't know how many malicious stuff you can do.

G

Better to inform antics from scratch, not reuse, because I.

H

Mean why, when we have same framework, because.

G

Because I I don't see the reason from the code of idea validate the past and if we have strict rules is valid only for far out I can written the stood while validated.

G

G

Maybe can you add to the proposals, the rules? What is by the trout, because you know any F drink- is very trout.

G

Is why I asked you to add their rules.

I

And also probably isn't validator wouldn't have its production ID.

A

Okay, so please add your.

A

H

I think one class that will validate you know configure closet, does your eyes and.

A

J

Okay, so I've been investigating.

J

Because looks like installation of.

J

J

Investigate what is the problem and what can be improved and it's a measurements so.

J

Let me focus with them, so I have a I, have a comparison of to the three installations vision: zero, where packages loaded in parallel.

J

Sequentially, as you can see, there is four or almost almost five minutes different when I just loaded in parallel. It's almost five minutes. I survived.

J

Seven seconds.

J

That's comparison to the three just outlawed in the Odeon packages, one of the difference between two the theories and we have 80 more packages.

J

Then I focused on trying to understand what are the differences between the different latigo and our other experiments have packages. I have packages cash, so it looks like in to the 3.0. The dependency resolution also takes more time and.

J

If you run, if you run a composite with no motive option, it doesn't have much impact of dependency resolution because compose are still needs to recalculate dependencies in composer. So this project, so we don't have a composer work in the project. Isis file composing is to calculate dependencies for.

J

Required and for required dev dependencies, this required their dependencies. If you remove magenta to functional testing framework from required, their dependencies into the 2.0 dependency resolution will be in seconds.

J

Not 50 spheres, so we needed more dependencies to require linear sections is why dependencies.

J

J

J

J

This, if you just remove this magenta.

J

J

The solutions that they found is use was a packet sent to us to don't work packages in parallel. This.

J

Boost I have proposed and this partying globally I'm sorry.

J

The combination list install this plugin globally. So with.

J

This plug-in to to make sure that people who tell bid documentation will get this benefits on on the next time when they do a composite update. The.

F

Same so for this, among several years ago, I was time to use it in weight and box and didn't work. So it's possible. It's not work for everybody. We need to probably extensive testing before encoding by default. The very concept which we.

J

I

Just just something to the discussion here: actually we did use this plugin on couple of occasions a couple of projects, and actually the results were quite the other way around. So generally, the timing to to install the project using composer, a great project and so on were actually better than than what we see here. Well, I didn't I, didn't do any. You know detailed or extend tests there, but you know from our perspective on some bigger projects. Yeah. The improvement was clearly visible in terms of timing of setting up a new projects using this one.

I

So I can try to ask one of my colleagues who was, let's have mainly responsible for doing this, to get some more details and shared that with you. If that will be helpful.

B

I

I'll try to get in touch with my colleague.

J

So another problem is.

J

Looks like is because of require that dependency is insensitive dependency, as many.

F

Other dependencies.

J

Like our testing free ones and dependency resolution, we don't take much more time, so one of the I have a couple of years to this.

J

So we can move these dependencies in a separate package, as this would mean that if.

J

Install a separate package versus dependencies so again require no, no their flag doesn't work because composers to operate some of those dependencies and create composable. So in this way everyone will get improvement and.

J

Another option is to add composer log file to the project problem is this solution? Is.

J

Some packages, for instance, PHP unit this one to get latest PHP unit and composed of ulsan. We will not get when dispersion of some of the packages most projects, CC opinions, don't oppose. It walk to the crotchet another option, which is in a complicated extent, proposer to fish composite work from from the server, and in this case we can like bypasses dependency resolution composite create project and, on our several make sure that composing work is always up to date. On his recent versions of of dependencies, we can updated.

F

Depending on environment installed on it Marisol to different values and the hawk who depend on exact environment, you use.

K

Anything involving a composer lock is very problematic one and just keeping it up to date, but to just environment dependencies. Exactly these points, I would strongly boat against anything involving the lock file.

J

Data application and the folder of files needs to be extracted and copied. This is we all probably get funny 30 seconds. I didn't measure this, but I.

J

Remember I was measuring composer extract time and there was something like you know, 20 seconds also we can make up application work from whether directories.

J

J

J

J

So this is I wanted to share with you.

K

Inherited by one of our dependencies, okay,.

A

A

We can try that as well, but the problem is that I think some of those solutions they help only.

A

The composition, yeah.

J

I was looking I said: I was looking at this plugin, but wasn't able to.

J

L

Yes, first question: why does composure team not merge, not packaged download.

A

Understand and composure to the zero. Yes.

L

So so, okay composure to the dear on. What's the problem with using.

J

It's not enough, it gives sixty percent of the services ovation.

J

J

L

So other question: so what's the dependency resolution so I see that is different times of the penetrator? So my understanding we spent in following.

L

The difference between.

L

J

J

Doesn't it's not big improvement and some files have been copied, for instance, as a project itself in complications have been copied, and this is where we spent additional 10 seconds and installations.

K

Question about updater: why do we have updater at the project level instead of its own.

F

Obviously, update composer from from my childhood self-image hundreds of days. Yes, don't.

J

You be independent application. We can move updater to a separate package.

A

H

There anything we can do with the composer plug-in that you developed Peter to some of these problems. No.

K

Not for initial project installation, the plug-in on the works on updating and actually the plug-in will increase initial project time because it has to install itself yeah.

K

It has to install itself and then install itself again in the VAR folder for for the web setup. Wizard.

A

J

K

Dependency resolution is not a linear scale, the more dependencies you have, the harder it is to resolve and it increases.

J

As far as they see.

J

Extra one minute, I, don't know what composure is doing, it doesn't exist, it increased this time increases I will try to understand where that time goes, but review solutions, comment on these options and I will try to see if we can another care packages in parallel. Okay,.

A