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

Remove underscore prefix in methods #14971

Merged
merged 7 commits into from
Jun 19, 2020
Merged

Conversation

Jeckerson
Copy link
Member

@Jeckerson Jeckerson commented May 1, 2020

Hello!

  • Type: new feature | code quality
  • Link to issue:

In raising this pull request, I confirm the following:

  • I have read and understood the Contributing Guidelines
  • I have checked that another pull request for this purpose does not exist
  • I wrote some tests for this PR
  • I have updated the relevant CHANGELOG
  • I have created a PR for the documentation about this change

Removed underscores in protected and extendable (not final) methods to be PSR-12 compliant.

Thanks

@Jeckerson Jeckerson requested review from niden, ruudboon and sergeyklay May 1, 2020 22:04
@Jeckerson Jeckerson self-assigned this May 1, 2020
@Jeckerson Jeckerson added the 4.1.0 label May 1, 2020
@sergeyklay
Copy link
Contributor

/cc @phalcon/core-team

@niden
Copy link
Member

niden commented May 3, 2020

We cannot do this now. It breaks backwards compatibility. If it was only private methods that would be fine but since it touches protected methods it will break applications that extend these classes and use those methods.

We can put a @todo on this for v5

@sergeyklay
Copy link
Contributor

Is there any place where we can start collect things for v5?

Copy link
Member

@niden niden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets make a small change here so that this can be merged safely without breaking BC.

Leave the underscore named methods where they are. Copy each of those methods to new ones without the underscore. From the one with the underscore, call the other one without. The underscore named methods become proxy ones. Add a @todo or @deprecated in the docblock of the underscore one so that we can remove those in v5.

This way we do what we need but also keep BC

@ruudboon
Copy link
Member

ruudboon commented Jun 8, 2020

@Jeckerson If you have time can you rebase and have look at the requested changes?

Copy link
Member

@ruudboon ruudboon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the BC methods

@Jeckerson
Copy link
Member Author

Will try to add it during today.

Copy link
Member

@ruudboon ruudboon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to suggest to implement to logic in the methods that are going to be in v5.
Now we are getting extra unnecessary look ups groupResult() calls _groupResult() and it will be easier when cleaning up the deprecated methods.

phalcon/Mvc/Model.zep Outdated Show resolved Hide resolved
@Jeckerson Jeckerson requested review from sergeyklay and ruudboon June 17, 2020 20:33
@niden niden added the enhancement Enhancement to the framework label Jun 19, 2020
@niden niden merged commit a1035b9 into 4.1.x Jun 19, 2020
@sergeyklay sergeyklay deleted the remove-methods-underscore-prefix branch July 6, 2020 05:49
@zsilbi
Copy link
Member

zsilbi commented Aug 4, 2020

I think this is going to break BC this way, because the logic is not quite right.

For example, if someone had custom a _postSaveRelatedRecords() implemented in their Model before, it's not going to be ever called by the Model itself after a save because it will only call the new function (postSaveRelatedRecords()) instead.

namespace My;

class Model extends \Phalcon\Mvc\Model {
   /**
    * My custom _postSaveRelatedRecords implementation
    */
   protected function _postSaveRelatedRecords(AdapterInterface $connection, $related): bool {
      // This won't be ever called by Model

      return parent::_postSaveRelatedRecords($connection, $related);
   }
}

Following this example, this solution only applies to those, who originally had Model::_postSaveRelatedRecords() calls in their Model for some reason.

namespace My;

class Model extends \Phalcon\Mvc\Model {
   /**
     * My custom function
     * 
     * @throws Exception
     * @throws Model\Exception
     */
    public function doSomething(): void
    {
        // This will work, thanks to the proxy function
        $this->_postSaveRelatedRecords(
            $this->getWriteConnection(),
            $this->dirtyRelated
        );
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement to the framework
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants