Re: [PHP-DEV] [RFC] noreturn type

[Matthew Brown]

> On Mar 29, 2021, at 1:25 PM, Ilija Tovilo  wrote:
> 
> Hi Matthew
> 
>> Ondřej Mirtes and I present an RFC for the noreturn type:
>> https://wiki.php.net/rfc/noreturn_type
>> 
>> The feature already exists in Hack (the primary inspiration) and is
>> currently supported by our static analysis tools inside docblocks, and we
>> feel there's a good argument for it to be supported by PHP itself.
> 
> Thanks for the RFC! I'm very much in support of it.
> 
> Two small things:
> 
> 1. Some magic methods like __toString currently require a specific
> return type (like string in that case). Since noreturn is a bottom
> type technically it should be possible to type hint those magic
> methods with noreturn. It's not a big issue if that's not possible,
> but it should be mentioned in the RFC.
> 
> 2. noreturn is one of the few return types that would technically make
> sense for __construct (other than void).
> 
> class Foo {
>public function __construct(): noreturn {
>throw new Exception();
>}
> }
> 
> new Foo();
> bar(); // < Dead code
> 
> Not sure this is worth supporting but I just wanted to mention it.
> 
> Ilija
> 
> -- 
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: https://www.php.net/unsub.php
> 

Thanks, I’ll update the RFC to mention __toString, but I’ll steer clear of 
__construct
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php

Re: [PHP-DEV] Changes to Git commit workflow

[Stanislav Malyshev]

Hi!

On 3/29/21 4:49 AM, Max Semenik wrote:

On Mon, Mar 29, 2021 at 1:53 AM Nikita Popov  wrote:


changes should be pushed directly to GitHub rather than to git.php.net.



Would it also make sense if direct pushes (bypassing the pull requests
system) were disallowed completely? I can see multiple problems with direct
pushes:


This is possible. In fact, there are Git bots that make it easier (e.g. 
prow: https://github.com/kubernetes/test-infra/tree/master/prow) - I am 
using such system over Github at my $DAYJOB and it's generally working 
well. It even has its own built-in karma-like system. However, it has 
some downsides, as the experience shows:


1. Quick management patches, typofixes, release management patches, etc. 
become more high friction processes.
2. Setup and configuration of such system involves some time investment 
by some knowledgeable people, and it has certain learning curve (though 
once it is set up, it's pretty easy to use).
3. Somebody knowledgeable needs to maintain it, as periodically bots can 
get stuck and need a gentle kick to continue.
4. CI needs to be very stable and clean for having CI pass as gateway to 
merge, otherwise a flaky test can block all work in the repo for days.

5. Managing multiple active branches can be a pain.

None of these are critical, and we could start small and build it 
incrementally, of course.


--
Stas Malyshev
smalys...@gmail.com

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php

[PHP-DEV] Re: Changes to Git commit workflow Nikita Popov

[Björn Larsson]

Den 2021-03-29 kl. 23:10, skrev Benjamin Morel:


Hi everyone,

Yesterday (2021-03-28) two malicious commits were pushed to the php-src
repo [1] from the names of Rasmus Lerdorf and myself. We don't yet know how
exactly this happened, but everything points towards a compromise of the
git.php.net server (rather than a compromise of an individual git
account).



That is scary. Can you disclose the contents of the commits? Are they
specially designed to open a security hole, or to be harmful in another way?

An article from The Hacker News and a tweet from Zerodium about the 
incident:

-https://thehackernews.com/2021/03/phps-git-server-hacked-to-insert-secret.html
-https://twitter.com/cBekrar/status/1376469666084757506

r//Björn L

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php

[PHP-DEV] Re: Changes to Git commit workflow

[Benjamin Morel]

>
> Hi everyone,
>
> Yesterday (2021-03-28) two malicious commits were pushed to the php-src
> repo [1] from the names of Rasmus Lerdorf and myself. We don't yet know how
> exactly this happened, but everything points towards a compromise of the
> git.php.net server (rather than a compromise of an individual git
> account).
>

That is scary. Can you disclose the contents of the commits? Are they
specially designed to open a security hole, or to be harmful in another way?


> While investigation is still underway, we have decided that maintaining
> our own git infrastructure is an unnecessary security risk, and that we
> will discontinue the git.php.net server. Instead, the repositories on
> GitHub, which were previously only mirrors, will become canonical. This
> means that changes should be pushed directly to GitHub rather than to
> git.php.net.
>

This change will be welcome anyway!

— Benjamin

Re: [PHP-DEV] [RFC] noreturn type

[Ilija Tovilo]

Hi Matthew

> Ondřej Mirtes and I present an RFC for the noreturn type:
> https://wiki.php.net/rfc/noreturn_type
>
> The feature already exists in Hack (the primary inspiration) and is
> currently supported by our static analysis tools inside docblocks, and we
> feel there's a good argument for it to be supported by PHP itself.

Thanks for the RFC! I'm very much in support of it.

Two small things:

1. Some magic methods like __toString currently require a specific
return type (like string in that case). Since noreturn is a bottom
type technically it should be possible to type hint those magic
methods with noreturn. It's not a big issue if that's not possible,
but it should be mentioned in the RFC.

2. noreturn is one of the few return types that would technically make
sense for __construct (other than void).

class Foo {
public function __construct(): noreturn {
throw new Exception();
}
}

new Foo();
bar(); // < Dead code

Not sure this is worth supporting but I just wanted to mention it.

Ilija

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php

Re: [PHP-DEV] [RFC] noreturn type

[Matthew Brown]

If there are no more questions, we plan to open up voting for this on Tuesday 
March 30. There will be two votes — a 2/3 majority required for the feature, 
and a simple majority required for the name — “noreturn” vs “never”.
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php

Re: [PHP-DEV] Changes to Git commit workflow

[G. P. B.]

On Mon, 29 Mar 2021 at 12:50, Max Semenik  wrote:

> On Mon, Mar 29, 2021 at 1:53 AM Nikita Popov  wrote:
>
> > changes should be pushed directly to GitHub rather than to git.php.net.
>
>
> Would it also make sense if direct pushes (bypassing the pull requests
> system) were disallowed completely? I can see multiple problems with direct
> pushes:
>
> 1) Someone trying to sneak in malicious code, like in the current incident
> (rarest but most damaging problem)
> 2) One dev pushes something, another dev disagrees, while the discussion
> continues, the potentially problematic commit stays on. Pre-merge reviews
> are much more natural for discussions.
> 3) Direct push bypasses CI to break tests, PRs can't be merged until the
> cause is identified and resolved (seems to be happening quite often)
>
> In my roughly one year around, I can now recall instances of each of these
> problems in php-src.
>
> --
> Best regards,
> Max Semenik
>

Although I agree that as a general rule many things should go through a PR,
I think making it mandatory is going overboard, there is no need for a PR
when
one does a simple type fix in a document or comment, or trivial
refactorings.
I also don't know how this would interact with bug fixes needing to be
merged
upwards or cherry picks from the security repo which most of us have (and
don't deserve) access to.

Moreover, we have some flaky tests so CI can break for no reason and the
CI matrix for master has a more complex nightly pipeline which does not run
in a PR and thus make certain issues only apparent after the merge.

IMHO making it required that direct pushes need to be signed commits is
a better way forward.

Best regards,

George P. Banyard

Re: [PHP-DEV] Changes to Git commit workflow

[Max Semenik]

On Mon, Mar 29, 2021 at 1:53 AM Nikita Popov  wrote:

> changes should be pushed directly to GitHub rather than to git.php.net.


Would it also make sense if direct pushes (bypassing the pull requests
system) were disallowed completely? I can see multiple problems with direct
pushes:

1) Someone trying to sneak in malicious code, like in the current incident
(rarest but most damaging problem)
2) One dev pushes something, another dev disagrees, while the discussion
continues, the potentially problematic commit stays on. Pre-merge reviews
are much more natural for discussions.
3) Direct push bypasses CI to break tests, PRs can't be merged until the
cause is identified and resolved (seems to be happening quite often)

In my roughly one year around, I can now recall instances of each of these
problems in php-src.

-- 
Best regards,
Max Semenik

Re: [PHP-DEV] [RFC] [Discussion] Adding return types to internal methods

[Máté Kocsis]

> For userland, there are already ways to declare the planned return type
> (aka @return in phpdoc), so we might not need any new way to declare this
> from the engine pov.
>

Yes, I think I agree, but I'd be curious about Nikita's opinion as well,
since he brought up this problem first.


> We should replace the SuppressReturnTypeNotice by the TentativeReturnType
> one right away.

The TentativeReturnType would mean: "I'm going to add a return type to this
> method in its next major".
> As a consequence, for child classes of internal methods, this would
> suppress the notice.
>

If we get rid of the userland part of my proposal, then I think we can
really rename SuppressReturnTypeNotice
to TentativeReturnType.


> As a corollary, adding this attribute should conflict with having a real
> return type.


This is the only questionable part for me, because then this RFC would
cause more trouble for overriding methods
which already define a wrong return type: a deprecation notice would always
be triggered for them on PHP 8.1+
until return types are fixed or removed. And as we discussed it previously,
fixing them is not always possible.
Or is there any specific reason I'm not aware of why you favor this
behavior?

Máté

Re: [PHP-DEV] Changes to Git commit workflow

[Paul Dragoonis]

On Mon, 29 Mar 2021, 08:51 Paul Dragoonis,  wrote:

>
>
> On Mon, 29 Mar 2021, 02:30 Rasmus Lerdorf,  wrote:
>
>> On Sun, Mar 28, 2021 at 17:15 Sara Golemon  wrote:
>>
>> > On Sun, Mar 28, 2021 at 6:57 PM Paul Crovella 
>> > wrote:
>> >
>> >> You might consider requiring commits be signed while you're at it.
>> >>
>> >>
>> > I suggested this as well, and even if we don't require it, we should
>> > STRONGLY encourage it.
>> >
>> > I've been signing my commits for several years now, it's not even that
>> > hard.
>> >
>> I think for php-src commits we can require it. For doc and other repos we
>> can make it optional for now until people are more comfortable with it.
>>
>
> Hey Rasmus,
>
> This is a good compromise.
>
> However, if you leave phpweb repo without signed commits then we're at
> risk from XSS or similar attacks still, and the surface area is really big
> because literally everyone is accessing the site.
>
> Many thanks,
> Paul
>

I also wanted to say; back when I was rebuilding our website a few years
ago, when you pushed to master it would automatically deploy this to the
live site.

If we are compromised and we still automatically roll out to production,
this would make it really easy for someone.

Can someone check how we currently do this, and maybe we should reconsider
auto production deploys, even if its temporary, to be on the safe side.


>
>
>
>> -Rasmus
>>
>

Re: [PHP-DEV] Changes to Git commit workflow

[Paul Dragoonis]

On Mon, 29 Mar 2021, 02:30 Rasmus Lerdorf,  wrote:

> On Sun, Mar 28, 2021 at 17:15 Sara Golemon  wrote:
>
> > On Sun, Mar 28, 2021 at 6:57 PM Paul Crovella 
> > wrote:
> >
> >> You might consider requiring commits be signed while you're at it.
> >>
> >>
> > I suggested this as well, and even if we don't require it, we should
> > STRONGLY encourage it.
> >
> > I've been signing my commits for several years now, it's not even that
> > hard.
> >
> I think for php-src commits we can require it. For doc and other repos we
> can make it optional for now until people are more comfortable with it.
>

Hey Rasmus,

This is a good compromise.

However, if you leave phpweb repo without signed commits then we're at risk
from XSS or similar attacks still, and the surface area is really big
because literally everyone is accessing the site.

Many thanks,
Paul




> -Rasmus
>

[PHP-DEV] Re: Changes to Git commit workflow

[Kalle Sommer Nielsen]

Den man. 29. mar. 2021 kl. 01.52 skrev Nikita Popov :
> While previously write access to repositories was handled through our 
> home-grown karma system, you will now need to be part of the php organization 
> on GitHub. If you are not part of the organization yet, or don't have access 
> to a repository you should have access to, contact me at ni...@php.net with 
> your php.net and GitHub account names, as well as the permissions you're 
> currently missing. Membership in the organization requires 2FA to be enabled.

How will this work for SVNROOT karma holders to grant karma to others
going forward?


-- 
regards,

Kalle Sommer Nielsen
ka...@php.net

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php

Re: [PHP-DEV] Changes to Git commit workflow

[Deleu]

I think you only need to handle merges locally if the pull request author
didn't sign their commits:

> You can always push local commits to the branch if the commits are signed
and verified.
> You can also merge signed and verified commits into the branch using a
pull request on GitHub.
> However, you cannot squash and merge a pull request into the branch on
GitHub unless you are the
> author of the pull request. You can squash and merge pull requests
locally. For more information, see
>  "Checking out pull requests locally

."

Source:
https://docs.github.com/en/github/administering-a-repository/about-protected-branches#require-signed-commits

On Mon, Mar 29, 2021, 06:19 Ayesh Karunaratne  wrote:

> I think this is a great step forward.
> With commit signatures required, I think the person who merges a PR
> now needs to do so locally. [GitHub CLI](https://cli.github.com/)
> helps me a lot to locally checkout a PR quickly, and then
> rebase/squash with my own signature.
>
> Thanks to Levi Morrison and Nikita for the very quick response.
>
> >
> > On Mon, 29 Mar 2021, 03:30 Rasmus Lerdorf,  wrote:
> >
> > > On Sun, Mar 28, 2021 at 17:15 Sara Golemon  wrote:
> > >
> > > > On Sun, Mar 28, 2021 at 6:57 PM Paul Crovella <
> paul.crove...@gmail.com>
> > > > wrote:
> > > >
> > > >> You might consider requiring commits be signed while you're at it.
> > > >>
> > > >>
> > > > I suggested this as well, and even if we don't require it, we should
> > > > STRONGLY encourage it.
> > > >
> > > > I've been signing my commits for several years now, it's not even
> that
> > > > hard.
> > > >
> > > I think for php-src commits we can require it. For doc and other repos
> we
> > > can make it optional for now until people are more comfortable with it.
> > >
> > > -Rasmus
> >
> >
> > >
> > > We can require Signed Commits for the main active branches on GitHub:
> > >
> https://docs.github.com/en/github/administering-a-repository/about-protected-branches#require-signed-commits
> >
> >
> > We can create rules that requires that for all active maintained version
> of
> > PHP.
> >
> > We can set that per repo, or in a organization level.
> >
> > - Gabriel Caruso
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: https://www.php.net/unsub.php
>
>